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

src/renderer/src/components/PassageDetail/PassageDetailGuidedPhraseRecord.tsx

@@ -1057,6 +1057,17 @@ export function PassageDetailGuidedPhraseRecord({
     handleRegionPlayEndRef.current(region);
   }, []);
 
+  /**
+   * A click on the waveform is a deliberate selection, so it can never be the
+   * playback overshoot the swallow exists to absorb. Disarm it here, before the
+   * segment change it produces reaches the navigation effect below — otherwise
+   * clicking the segment right after the current one is indistinguishable from
+   * overshoot and gets eaten, leaving the user's first click with no effect.
+   */
+  const handleSegmentClick = useCallback(() => {
+    pendingOvershootSwallowRef.current = false;
+  }, []);
+
   useEffect(() => {
     if (!bootstrapped || !entryPositioned || entryPauseDoneRef.current) return;
     if (recordingPassStarted) return; // recording pass auto-plays on entry; don't pause
@@ -1684,6 +1695,7 @@ export function PassageDetailGuidedPhraseRecord({
           controlsRef={playerControlsRef}
           applyRegionColor={applyRegionColor}
           onSegmentPlaybackEnd={onSegmentPlaybackEnd}
+          onSegmentClick={handleSegmentClick}
           highlightPlay={highlightPlayButton}
           onPlayStatusNotify={handlePlayStatusNotify}
           beforePlay={handleBeforeSourcePlay}

src/renderer/src/components/PassageDetail/PassageDetailPhraseBackTranslate.selection.cy.tsx

@@ -54,37 +54,41 @@ describe('PBT waveform segment selection', () => {
     unitLabel('0:06', '0:09').should('be.visible');
   });
 
-  it(
-    'DEFECT: the first click on the very next segment is ignored',
-    { tags: '@known-defect' },
-    () => {
-      // After a segment finishes playing, handleRegionPlayEnd arms
-      // pendingOvershootSwallowRef so the +1 segment change that playback
-      // overshoot produces can be absorbed. It cannot tell that change apart from
-      // the user clicking the next segment, so the click is swallowed too: the
-      // playhead snaps back, the label never changes, and the user has to click
-      // again. Record also stays enabled for the segment they were leaving.
-      clickSegmentOnWaveform(1);
-      unitLabel('0:03', '0:06').should('be.visible');
-    }
-  );
+  it('acts on the first click even on the very next segment', () => {
+    // After a segment finishes playing, handleRegionPlayEnd arms
+    // pendingOvershootSwallowRef so the +1 segment change that playback
+    // overshoot produces can be absorbed. It could not tell that change apart
+    // from the user clicking the next segment, so the click was swallowed too:
+    // the playhead snapped back, the label never changed, and the user had to
+    // click again. The player now reports a click distinctly, which disarms the
+    // swallow.
+    clickSegmentOnWaveform(1);
+    unitLabel('0:03', '0:06').should('be.visible');
+  });
 
   it('keeps Record off while a clicked segment plays', () => {
     clickSegmentOnWaveform(2);
     unitLabel('0:06', '0:09').should('be.visible');
 
-    sampleDom(
-      (doc) => ({
-        playing: readSourcePlaying(doc),
-        record: readRecordEnabled(doc),
-      }),
-      { forMs: 6000 }
-    ).then((samples) => {
-      const bothLive = samples.filter((s) => s.playing && s.record);
+    // One reading, taken from the middle of the segment. `playing` is the
+    // player's own state and Record's operability is the step's, so at either
+    // end of playback the two flip on unrelated renders and a sample can
+    // legitimately catch both live for a frame. Segment 3 runs 0:06-0:09, so
+    // waiting for playback to start and then settling for most of a second
+    // lands clear of both edges. Asserting `playing` in the same reading is
+    // what makes the Record assertion mean anything.
+    cy.document().should((doc) => {
+      expect(readSourcePlaying(doc), 'reference audio started').to.equal(true);
+    });
+    cy.wait(800);
+    cy.document().then((doc) => {
+      const playing = readSourcePlaying(doc);
+      const record = readRecordEnabled(doc);
+      expect(playing, 'reference audio still playing').to.equal(true);
       expect(
-        bothLive,
-        'Record was never operable while the reference audio played'
-      ).to.have.length(0);
+        record,
+        'Record operable while the reference audio plays'
+      ).to.equal(false);
     });
   });
 });
@@ -106,7 +110,8 @@ describe('PBT segment selection after a take exists', () => {
       // during playing which should not be possible". Recording over the
       // reference audio is what the listen-then-record flow prevents everywhere
       // else, and Record is correctly off for this same click before any take
-      // exists (see the previous describe).
+      // exists (see the previous describe). Fixed separately - this change only
+      // stops the click itself being swallowed.
       clickSegmentOnWaveform(2);
 
       sampleDom(
@@ -130,10 +135,10 @@ describe('PBT segment selection after a take exists', () => {
     { tags: '@known-defect' },
     () => {
       // Reported as "the yellow highlighting briefly jumps to the next segment".
-      // The waveform paints from the engine's currentSegmentIndex while the label
-      // comes from the step's own currentIndex; when a click only reaches the
-      // engine, the two disagree - briefly in the good case, indefinitely in the
-      // one below.
+      // The waveform paints from the engine's currentSegmentIndex while the
+      // label comes from the step's own currentIndex, so any segment change the
+      // step does not act on shows up as the two disagreeing. Fixed separately -
+      // this change only stops the click itself being swallowed.
       clickSegmentOnWaveform(2);
 
       sampleDom(
@@ -188,25 +193,37 @@ describe('PBT recording out of order (1, 3, then 2)', () => {
     });
   });
 
-  it(
-    'DEFECT: clicking back to segment 2 leaves the step on segment 3',
-    { tags: '@known-defect' },
-    () => {
-      // Reported: "I record the first segment, then the third, and then try to
-      // go back and record the second, it records into and replaces the third".
-      // Clicking segment 2 moves the waveform selection and the playhead there,
-      // but the step stays on segment 3 - so the next take is filed under
-      // segment 3, on top of the take already there. Asserting the label is
-      // enough: while it still reads segment 3, anything recorded goes to the
-      // wrong segment.
-      recordAndSettle(1); // segment 1
+  it('follows a click back to an earlier segment and files the take there', () => {
+    // Reported: "I record the first segment, then the third, and then try to go
+    // back and record the second, it records into and replaces the third".
+    // The click moved the waveform selection to segment 2 while the step stayed
+    // on segment 3, so the take was filed on segment 3 over the one already
+    // there. The engine reports a 1-based segment index and the step sets a
+    // 0-based one, so this move arrived carrying the index the step had just
+    // written (engine 1+1 vs step 2) and the navigation effect never re-ran.
+    recordAndSettle(1); // segment 1
 
-      clickSegmentOnWaveform(2);
-      unitLabel('0:06', '0:09').should('be.visible');
-      recordAndSettle(2); // segment 3
+    clickSegmentOnWaveform(2);
+    unitLabel('0:06', '0:09').should('be.visible');
+    recordAndSettle(2); // segment 3
 
-      clickSegmentOnWaveform(1);
-      unitLabel('0:03', '0:06').should('be.visible');
-    }
-  );
+    clickSegmentOnWaveform(1);
+    unitLabel('0:03', '0:06').should('be.visible');
+    recordTake();
+    waitForUploads(3);
+
+    cy.then(() => {
+      const segs = postedTakes().map((t) => t.parsedSegments);
+      expect(segs[2], 'take recorded on segment 2 lands there').to.deep.include(
+        {
+          start: 3,
+          end: 6,
+        }
+      );
+      const onSegment3 = segs.filter(
+        (s) => s?.start === 6 && s?.end === 9
+      ).length;
+      expect(onSegment3, 'segment 3 still has exactly one take').to.equal(1);
+    });
+  });
 });

src/renderer/src/components/PassageDetail/PassageDetailPlayer.tsx

@@ -103,6 +103,8 @@ export interface DetailPlayerProps {
   /** Tool-specific waveform region coloring (Mark Verses, Careful Speech, etc.). */
   applyRegionColor?: ApplyRegionColor;
   onSegmentPlaybackEnd?: (region: IRegion) => void;
+  /** A segment was clicked on the waveform (not selected by the playhead). */
+  onSegmentClick?: (region: IRegion) => void;
   /** Called when waveform play/pause changes (in addition to internal player logic). */
   onPlayStatusNotify?: (playing: boolean) => void;
   highlightPlay?: boolean;
@@ -155,6 +157,7 @@ export function PassageDetailPlayer(props: DetailPlayerProps) {
     controlsRef,
     applyRegionColor,
     onSegmentPlaybackEnd,
+    onSegmentClick,
     onPlayStatusNotify,
     highlightPlay,
     playerState,
@@ -441,6 +444,7 @@ export function PassageDetailPlayer(props: DetailPlayerProps) {
         controlsRef={controlsRef}
         applyRegionColor={applyRegionColor}
         onSegmentPlaybackEnd={onSegmentPlaybackEnd}
+        onSegmentClick={onSegmentClick}
         blob={audioBlob}
         initialposition={initialposition}
         setInitialPosition={setInitialPosition}

src/renderer/src/components/WSAudioPlayer.tsx

@@ -175,6 +175,8 @@ interface IProps {
     index?: number
   ) => void;
   onSegmentPlaybackEnd?: (segment: IRegion) => void;
+  /** A segment was clicked on the waveform (not selected by the playhead). */
+  onSegmentClick?: (segment: IRegion) => void;
   forceRegionOnly?: boolean;
   /** When true, user-initiated selection (clicks, prev/next) is ignored. Playhead-driven region-in is not blocked; consumers that must hold the current clause during recording/saving should guard their segment effects separately (e.g. Careful Speech). */
   lockSegmentSelection?: boolean;
@@ -366,6 +368,7 @@ function WSAudioPlayer(props: IProps) {
     onRecording,
     onCurrentSegment,
     onSegmentPlaybackEnd,
+    onSegmentClick,
     forceRegionOnly,
     lockSegmentSelection,
     onMarkerClick,
@@ -718,7 +721,8 @@ function WSAudioPlayer(props: IProps) {
     verses,
     hasSegmentUndo,
     applyRegionColor,
-    lockSegmentSelection
+    lockSegmentSelection,
+    onSegmentClick
   );
 
   //because we have to call hooks consistently, call this even if we aren't going to record

src/renderer/src/crud/useWaveSurfer.tsx

@@ -61,7 +61,9 @@ export function useWaveSurfer(
   verses?: string,
   hasSegmentUndo?: boolean,
   applyRegionColor?: ApplyRegionColor,
-  lockSegmentSelection?: boolean
+  lockSegmentSelection?: boolean,
+  /** A region was clicked, as opposed to selected by the playhead. */
+  onSegmentClick?: (region: IRegion) => void
 ) {
   const { isMobile } = useMobile();
   const [errorReporter] = useGlobal('errorReporter');
@@ -308,7 +310,8 @@ export function useWaveSurfer(
     hasSegmentUndo,
     applyRegionColor,
     lockSegmentSelection,
-    () => blobAudioRef.current
+    () => blobAudioRef.current,
+    onSegmentClick
   );
 
   const setPlayingx = (value: boolean, regionOnly: boolean) => {

src/renderer/src/crud/useWavesurferRegions.tsx

@@ -124,7 +124,12 @@ export function useWaveSurferRegions(
   hasSegmentUndo?: boolean,
   applyRegionColor?: ApplyRegionColor,
   lockSegmentSelection?: boolean,
-  getDecodedBuffer?: () => AudioBuffer | undefined
+  getDecodedBuffer?: () => AudioBuffer | undefined,
+  /**
+   * A region was clicked. Distinct from onCurrentRegion, which also fires for
+   * playhead-driven selection: only a deliberate user click reaches this.
+   */
+  onRegionClicked?: (region: IRegion) => void
 ) {
   const theme = useTheme();
   const wsRef = useRef<WaveSurfer | null>(ws);
@@ -284,6 +289,14 @@ export function useWaveSurferRegions(
     } else {
       setCurrentRegion(r);
       if (!wasCurrentRegion) {
+        // Only a click that actually changes the selection counts as deliberate
+        // navigation. Clicking the already-current region is a no-op that must
+        // not disarm a pending overshoot swallow (see onRegionClicked consumers).
+        onRegionClicked?.({
+          start: r.start,
+          end: r.end,
+          label: r.content?.textContent || '',
+        });
         goto(r.start, false, { start: r.start, end: r.end });
         e.stopPropagation();
       }