From 03175fcc1cc157f7e3b90eaf24bc411fc67ad5c7 Mon Sep 17 00:00:00 2001 From: Noel Chou Date: Wed, 2 Sep 2026 17:21:45 -0400 Subject: [PATCH 01/27] TT-7437 tests: a take belongs to the segment it started on MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit 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) --- .../PassageDetailCarefulSpeech.test.tsx | 91 +++++- .../src/crud/useWavesurferRegions.test.tsx | 285 ++++++++++++++++++ 2 files changed, 368 insertions(+), 8 deletions(-) create mode 100644 src/renderer/src/crud/useWavesurferRegions.test.tsx diff --git a/src/renderer/src/components/PassageDetail/PassageDetailCarefulSpeech.test.tsx b/src/renderer/src/components/PassageDetail/PassageDetailCarefulSpeech.test.tsx index a1323a799..9efa10424 100644 --- a/src/renderer/src/components/PassageDetail/PassageDetailCarefulSpeech.test.tsx +++ b/src/renderer/src/components/PassageDetail/PassageDetailCarefulSpeech.test.tsx @@ -208,8 +208,7 @@ import { PassageDetailCarefulSpeech } from './PassageDetailCarefulSpeech'; // Fire the player's region-out callback for a given clause. const firePlaybackEnd = async (idx: number) => { const cb = playerProps?.onSegmentPlaybackEnd as - | ((r: IRegion) => void) - | undefined; + ((r: IRegion) => void) | undefined; await act(async () => { cb?.(regions[idx]); }); @@ -317,8 +316,7 @@ describe('PassageDetailCarefulSpeech — review and clear recording', () => { await mountAndSettle(); const onClearRecording = controlsProps?.onClearRecording as - | (() => void) - | undefined; + (() => void) | undefined; await act(async () => { onClearRecording?.(); }); @@ -439,6 +437,85 @@ describe('PassageDetailCarefulSpeech — recording segment lock (TT-7437)', () = }); }); +describe('PassageDetailCarefulSpeech — take belongs to the clause it started on (TT-7437)', () => { + // Record a take on clause 2 and hand back the sourceSegments value the + // recorder must file it under. + const startTakeOnClause2 = async () => { + mockCompleted = new Set([0, 1]); // auto-play clause 2 + const utils = await mountAndSettle(); + await firePlaybackEnd(2); // park on clause 2 + + expect(controlsProps?.sourceSegments).toBe(JSON.stringify(regions[2])); + + await act(async () => { + (controlsProps?.onRecording as (active: boolean) => void)(true); + }); + return utils; + }; + + it('keeps the take on its clause when the selection moves mid-record', async () => { + const { rerender } = await startTakeOnClause2(); + + await moveEngineToClause(6, rerender); + + expect(controlsProps?.sourceSegments).toBe(JSON.stringify(regions[2])); + }); + + it('keeps the take on its clause when the selection change lands after stop', async () => { + // The waveform click is dropped while locked, but the seek it caused makes + // the playhead enter the tapped clause; that region-in can arrive in the + // gap between the recorder stopping and the save starting. The take was + // already made — it still belongs to clause 2 (TT-7437). + const { rerender } = await startTakeOnClause2(); + + await act(async () => { + (controlsProps?.onRecording as (active: boolean) => void)(false); + }); + await moveEngineToClause(6, rerender); + + expect(controlsProps?.sourceSegments).toBe(JSON.stringify(regions[2])); + }); + + it('keeps the take on its clause while the save is in flight', async () => { + const { rerender } = await startTakeOnClause2(); + + await act(async () => { + (controlsProps?.onRecording as (active: boolean) => void)(false); + }); + await act(async () => { + (controlsProps?.setCanSave as (v: boolean) => void)(true); + }); + await moveEngineToClause(6, rerender); + + expect(controlsProps?.sourceSegments).toBe(JSON.stringify(regions[2])); + }); + + it('greens the clause it recorded, not the one the selection moved to', async () => { + const { rerender } = await startTakeOnClause2(); + + const colorOf = (index: number) => + ( + playerProps?.applyRegionColor as + ((role: string, index: number, count: number) => string) | undefined + )?.('base', index, 8); + + await act(async () => { + (controlsProps?.onRecording as (active: boolean) => void)(false); + }); + await moveEngineToClause(6, rerender); + await act(async () => { + await ( + controlsProps?.afterUploadCb as ( + mediaId: string | undefined + ) => Promise + )('media-new'); + }); + + expect(colorOf(2)).toBe(CAREFUL_SPEECH_COMPLETED_RGBA); + expect(colorOf(6)).toBe(CAREFUL_SPEECH_PENDING_RGBA); + }); +}); + describe('PassageDetailCarefulSpeech — segment change after take (TT-7552)', () => { it('after saving a take, tapping the next clause advances and resets the recorder', async () => { mockCompleted = new Set([0, 1]); // auto-play clause 2 @@ -479,8 +556,7 @@ describe('PassageDetailCarefulSpeech — segment change after take (TT-7552)', ( const applyRegionColor = () => ( playerProps?.applyRegionColor as - | ((role: string, index: number, count: number) => string) - | undefined + ((role: string, index: number, count: number) => string) | undefined )?.('base', 2, 8); expect(applyRegionColor()).toBe(CAREFUL_SPEECH_PENDING_RGBA); @@ -511,8 +587,7 @@ describe('PassageDetailCarefulSpeech — segment change after take (TT-7552)', ( const applyRegionColor = () => ( playerProps?.applyRegionColor as - | ((role: string, index: number, count: number) => string) - | undefined + ((role: string, index: number, count: number) => string) | undefined )?.('base', 2, 8); expect(applyRegionColor()).toBe(CAREFUL_SPEECH_PENDING_RGBA); diff --git a/src/renderer/src/crud/useWavesurferRegions.test.tsx b/src/renderer/src/crud/useWavesurferRegions.test.tsx new file mode 100644 index 000000000..ac0af0923 --- /dev/null +++ b/src/renderer/src/crud/useWavesurferRegions.test.tsx @@ -0,0 +1,285 @@ +import { act, renderHook } from '@testing-library/react'; + +/** + * Segment-selection lock spec (TT-7437). + * + * While a take is being recorded the selected segment must not move: the take + * belongs to the segment recording started on. Three engine events can move it + * and each one is a separate route the user can take from the waveform: + * + * - `region-clicked` — tapping another segment, + * - `region-in` — the playhead entering another segment after the tap + * seeks it (the click also seeks, so this fires even + * when the click itself was dropped), + * - `region-updated` — dragging a segment boundary. + * + * All three land on `onCurrentRegion`, which is what drives + * PassageDetailContext's currentSegment. So the lock is asserted there rather + * than on any one handler's internals. + */ + +// ---- fake wavesurfer regions plugin ----------------------------------------- +type Handler = (...args: any[]) => void; + +/** The bits of the regions plugin the hook uses, plus an emit for the tests. */ +interface IFakePlugin { + regionList: any[]; + on(evt: string, cb: Handler): void; + unAll(): void; + emit(evt: string, ...args: any[]): void; + getRegions(): any[]; + addRegion(r: any): any; + enableDragSelection(): void; +} + +// The hook picks its plugin out of ws.getActivePlugins() with +// `instanceof RegionsPlugin`, so the fake has to *be* the mocked class. +jest.mock('wavesurfer.js/dist/plugins/regions', () => { + class FakeRegionsPlugin { + handlers: Record = {}; + regionList: any[] = []; + + on(evt: string, cb: Handler) { + (this.handlers[evt] ||= []).push(cb); + } + unAll() { + this.handlers = {}; + } + emit(evt: string, ...args: any[]) { + (this.handlers[evt] ?? []).forEach((cb) => cb(...args)); + } + getRegions() { + return this.regionList; + } + addRegion(r: any) { + this.regionList.push(r); + return r; + } + enableDragSelection = jest.fn(); + } + return { __esModule: true, default: FakeRegionsPlugin }; +}); +jest.mock('wavesurfer.js', () => ({ __esModule: true, default: class {} })); + +// imported after the mocks so the hook picks them up +import RegionsPlugin from 'wavesurfer.js/dist/plugins/regions'; +import { useWaveSurferRegions } from './useWavesurferRegions'; + +const newPlugin = () => + new (RegionsPlugin as unknown as new () => IFakePlugin)(); + +const makeRegion = (id: string, start: number, end: number) => { + const r: any = { + id, + start, + end, + color: '', + drag: false, + resize: false, + content: undefined, + element: undefined, + attributes: {}, + setOptions: jest.fn((opts: any) => Object.assign(r, opts)), + play: jest.fn(), + remove: jest.fn(), + }; + return r; +}; + +const DURATION = 30; + +interface IHarnessOpts { + lockSegmentSelection: boolean; +} + +const renderRegions = ({ lockSegmentSelection }: IHarnessOpts) => { + const plugin = newPlugin(); + // three contiguous segments, linked the way setPrevNext links them + const segs = [ + makeRegion('r0', 0, 10), + makeRegion('r1', 10, 20), + makeRegion('r2', 20, 30), + ]; + segs.forEach((r, i) => { + r.attributes.prevRegion = segs[i - 1]; + r.attributes.nextRegion = segs[i + 1]; + }); + plugin.regionList = segs; + + const ws: any = { + getActivePlugins: () => [plugin], + on: jest.fn(), + un: jest.fn(), + play: jest.fn(), + getCurrentTime: () => 0, + }; + + const onCurrentRegion = jest.fn(); + const onRegionClicked = jest.fn(); + const goto = jest.fn(); + const onRegion = jest.fn(); + const setPlaying = jest.fn(); + + const { result } = renderHook(() => + useWaveSurferRegions( + false, // singleRegionOnly — Careful Speech is multi-region + 0, + ws, + { current: undefined }, + onRegion, + () => DURATION, + () => false, + goto, + () => 0, + () => false, + setPlaying, + onCurrentRegion, + undefined, // onStartRegion + undefined, // onRegionPlayEnd + undefined, // onMarkerClick + undefined, // verses + undefined, // hasSegmentUndo + undefined, // applyRegionColor + lockSegmentSelection, + undefined, // getDecodedBuffer + true, // disableDragSelection + onRegionClicked + ) + ); + + act(() => { + result.current.setupRegions(ws); + }); + + return { + result, + plugin, + segs, + onCurrentRegion, + onRegionClicked, + goto, + setPlaying, + }; +}; + +// The user taps a segment on the waveform. +const clickSegment = (plugin: IFakePlugin, r: any) => + act(() => { + plugin.emit('region-clicked', r, { stopPropagation: jest.fn() }); + }); + +// The playhead crosses into a segment (what the tap's seek causes next). +const playheadEnters = (plugin: IFakePlugin, r: any) => + act(() => { + plugin.emit('region-in', r); + }); + +// The user drags a segment boundary and lets go. 'region-update' is what tells +// the hook a resize (rather than a whole-region move) is underway. +const dragBoundary = ( + plugin: IFakePlugin, + r: any, + side: 'start' | 'end', + to: number +) => + act(() => { + r.resize = true; + plugin.emit('region-update', r, side); + r[side] = to; + plugin.emit('region-updated', r, side); + }); + +describe('useWaveSurferRegions — segment selection unlocked', () => { + it('a click selects the segment', () => { + const { plugin, segs, onCurrentRegion, onRegionClicked, goto } = + renderRegions({ lockSegmentSelection: false }); + + clickSegment(plugin, segs[2]); + + expect(onRegionClicked).toHaveBeenCalledWith( + expect.objectContaining({ start: 20, end: 30 }) + ); + expect(onCurrentRegion).toHaveBeenCalledWith({ start: 20, end: 30 }, 2); + expect(goto).toHaveBeenCalled(); + }); + + it('the playhead entering a segment selects it', () => { + const { plugin, segs, onCurrentRegion } = renderRegions({ + lockSegmentSelection: false, + }); + + playheadEnters(plugin, segs[1]); + + expect(onCurrentRegion).toHaveBeenCalledWith({ start: 10, end: 20 }, 1); + }); + + it('dragging a boundary reports the resized segment', () => { + const { plugin, segs, onCurrentRegion } = renderRegions({ + lockSegmentSelection: false, + }); + + dragBoundary(plugin, segs[1], 'end', 22); + + expect(onCurrentRegion).toHaveBeenCalledWith( + expect.objectContaining({ start: 10 }), + 1 + ); + }); +}); + +describe('useWaveSurferRegions — segment selection locked while recording (TT-7437)', () => { + it('drops a click on another segment', () => { + const { plugin, segs, onCurrentRegion, onRegionClicked, goto } = + renderRegions({ lockSegmentSelection: true }); + + clickSegment(plugin, segs[2]); + + expect(onRegionClicked).not.toHaveBeenCalled(); + expect(onCurrentRegion).not.toHaveBeenCalled(); + // no seek either: seeking is what makes the playhead enter the segment + expect(goto).not.toHaveBeenCalled(); + }); + + it('does not follow the playhead into another segment', () => { + // The click above is dropped, but a tap on the waveform also seeks. If the + // seek lands in another segment, region-in fires with the lock none the + // wiser — this is the route that kept moving the selection mid-record. + const { plugin, segs, onCurrentRegion } = renderRegions({ + lockSegmentSelection: true, + }); + + playheadEnters(plugin, segs[2]); + + expect(onCurrentRegion).not.toHaveBeenCalled(); + }); + + it('does not move the selection when a boundary is dragged', () => { + const { plugin, segs, onCurrentRegion } = renderRegions({ + lockSegmentSelection: true, + }); + + dragBoundary(plugin, segs[1], 'end', 22); + + expect(onCurrentRegion).not.toHaveBeenCalled(); + }); + + it('leaves the segment bounds alone when a boundary is dragged', () => { + // Dragging must not silently reshape the segment the take is being + // recorded into either — the take's start/end are what identify it. + const { plugin, segs } = renderRegions({ lockSegmentSelection: true }); + + dragBoundary(plugin, segs[1], 'end', 22); + + expect(segs[2].start).toBe(20); + expect(segs[1].setOptions).not.toHaveBeenCalledWith( + expect.objectContaining({ end: expect.anything() }) + ); + }); + + it('refuses prev/next segment navigation', () => { + const { result } = renderRegions({ lockSegmentSelection: true }); + + expect(result.current.wsPrevRegion()).toBe(false); + expect(result.current.wsNextRegion()).toBe(false); + }); +}); From c2d47d17fb388b2519718d8863d2cc9691b5e776 Mon Sep 17 00:00:00 2001 From: Noel Chou Date: Wed, 2 Sep 2026 17:33:14 -0400 Subject: [PATCH 02/27] TT-7437 file a take on the segment it was recorded on MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit 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) --- .../PassageDetailGuidedPhraseRecord.tsx | 80 ++++++++++++++++--- .../src/crud/useWavesurferRegions.tsx | 50 +++++++++++- 2 files changed, 118 insertions(+), 12 deletions(-) diff --git a/src/renderer/src/components/PassageDetail/PassageDetailGuidedPhraseRecord.tsx b/src/renderer/src/components/PassageDetail/PassageDetailGuidedPhraseRecord.tsx index bf638087a..bf5fe105f 100644 --- a/src/renderer/src/components/PassageDetail/PassageDetailGuidedPhraseRecord.tsx +++ b/src/renderer/src/components/PassageDetail/PassageDetailGuidedPhraseRecord.tsx @@ -261,6 +261,31 @@ export function PassageDetailGuidedPhraseRecord({ /** Indices saved this session whose rowData may not have caught up yet (TT-7552). */ const optimisticCompletedRef = useRef>(new Set()); const currentIndexRef = useRef(0); + /** + * The clause a take was started on, latched when capture begins and held + * until the take is stored or discarded (TT-7437). + * + * Everything that files a take — sourceSegments, the filename postfix, the + * green completion mark — used to read the *live* selection at save time, so + * any path that moved the selection between Record and the upload misfiled + * the audio. The engine-side lock stops the known routes, but a take belongs + * to the clause it was recorded on whatever slips through, so that clause is + * captured once and read back from here. + */ + const [recordingTarget, setRecordingTarget] = useState< + { index: number; region: IRegion } | undefined + >(undefined); + // Mirror for the upload callbacks, which fire outside a render. + const recordingTargetRef = useRef< + { index: number; region: IRegion } | undefined + >(undefined); + const latchRecordingTarget = useCallback( + (target: { index: number; region: IRegion } | undefined) => { + recordingTargetRef.current = target; + setRecordingTarget(target); + }, + [] + ); const [heardIndices, setHeardIndices] = useState([]); const [currentClausePlayed, setCurrentClausePlayed] = useState(false); const [combineUndo, setCombineUndo] = useState(null); @@ -531,8 +556,13 @@ export function PassageDetailGuidedPhraseRecord({ [canDoSectionStep, currentstep, section, passage, sharedResource] ); + /** The clause a pending take is filed under: latched at Record if there is + * one, otherwise wherever the user is now. */ + const takeIndex = recordingTarget?.index ?? currentIndex; + const takeRegion = recordingTarget?.region ?? currentRegion; + const defaultFilename = useMemo(() => { - const postfix = config.buildFilenamePostfix(currentIndex, currentVersion); + const postfix = config.buildFilenamePostfix(takeIndex, currentVersion); return passageDefaultFilename( passage, plan, @@ -547,7 +577,7 @@ export function PassageDetailGuidedPhraseRecord({ memory, artifactTypeId, offline, - currentIndex, + takeIndex, currentVersion, config, ]); @@ -678,8 +708,14 @@ export function PassageDetailGuidedPhraseRecord({ // Keyed on the index rather than the navigation handlers because every clause // move funnels through it. useEffect(() => { + // Navigating away from a failed take abandons it, so it gives up its + // latched clause with the message (TT-7437). Only that case: a take that + // is recording, uploading, or waiting to upload keeps its clause however + // the selection moves — that is the whole point of the latch. + if (saveRejectedRef.current) latchRecordingTarget(undefined); saveRejectedRef.current = false; setSaveRejected(false); + // eslint-disable-next-line react-hooks/exhaustive-deps }, [currentIndex]); useEffect(() => { @@ -1713,10 +1749,19 @@ export function PassageDetailGuidedPhraseRecord({ // (TT-7552). Only on a real upload though — a terminal failure still calls // us, with no mediaId, and painting that green tells the user their take // was stored when it was not (TT-7583). + // The clause the take was recorded on, not wherever the selection has + // since ended up — the green mark has to land where the audio went + // (TT-7437). + const takenIndex = + recordingTargetRef.current?.index ?? currentIndexRef.current; if (mediaId) { - optimisticCompletedRef.current.add(currentIndexRef.current); + optimisticCompletedRef.current.add(takenIndex); + // Stored: the take is no longer pending, so release the clause. + latchRecordingTarget(undefined); } else { - optimisticCompletedRef.current.delete(currentIndexRef.current); + optimisticCompletedRef.current.delete(takenIndex); + // Keep the latch on a failed upload: Retry must file the take on the + // same clause, however far the user has wandered (TT-7583). } // Stays 'recorded' either way: the take still exists, it just is not // stored. That keeps Record disabled and the clear button available, so @@ -1728,7 +1773,7 @@ export function PassageDetailGuidedPhraseRecord({ setResetMedia(false); applyColors(); }, - [forceRefresh, applyColors] + [forceRefresh, applyColors, latchRecordingTarget] ); const handleClearRecording = useCallback(async () => { @@ -1749,7 +1794,11 @@ export function PassageDetailGuidedPhraseRecord({ await setStepComplete(currentstep, false); } } - optimisticCompletedRef.current.delete(currentIndexRef.current); + optimisticCompletedRef.current.delete( + recordingTargetRef.current?.index ?? currentIndexRef.current + ); + // The take is gone, so the clause it was held against is released too. + latchRecordingTarget(undefined); setPhase('recordReady'); setCurrentClausePlayed(true); setResetMedia(true); @@ -1944,7 +1993,7 @@ export function PassageDetailGuidedPhraseRecord({ passageId={related(playerMediafile, 'passage') ?? passage?.id} artifactId={artifactTypeId} sourceMediaId={mediafileId} - sourceSegments={JSON.stringify(currentRegion ?? {})} + sourceSegments={JSON.stringify(takeRegion ?? {})} languagebcp47={stepLanguageField} defaultFilename={defaultFilename} recordingMediaId={recordingRow?.mediafile?.id} @@ -1952,6 +2001,16 @@ export function PassageDetailGuidedPhraseRecord({ onRecording={(active) => { if (active) { recordingActiveRef.current = true; + // Latch the clause this take belongs to. Everything that files + // the take reads it from here, so the audio lands where it was + // recorded no matter what moves the selection afterwards + // (TT-7437). + if (currentRegion) { + latchRecordingTarget({ + index: currentIndex, + region: currentRegion, + }); + } // A new take supersedes any earlier rejected save (TT-7583). saveRejectedRef.current = false; setSaveRejected(false); @@ -1983,8 +2042,11 @@ export function PassageDetailGuidedPhraseRecord({ setSavingRecording(false); // Upload failures route through afterUploadCb('') as well, but // MediaRecord's save-requested-with-no-audio branch only lands here, - // so undo the optimistic green from this path too (TT-7583). - optimisticCompletedRef.current.delete(currentIndexRef.current); + // so undo the optimistic green from this path too (TT-7583). The + // latch stays up so a Retry still files the take on its own clause. + optimisticCompletedRef.current.delete( + recordingTargetRef.current?.index ?? currentIndexRef.current + ); applyColors(); }} setStatusText={setStatusText} diff --git a/src/renderer/src/crud/useWavesurferRegions.tsx b/src/renderer/src/crud/useWavesurferRegions.tsx index 44d675568..c6a47113c 100644 --- a/src/renderer/src/crud/useWavesurferRegions.tsx +++ b/src/renderer/src/crud/useWavesurferRegions.tsx @@ -161,6 +161,11 @@ export function useWaveSurferRegions( applyRegionColor ); const lockSegmentSelectionRef = useRef(lockSegmentSelection ?? false); + /** Regions whose drag/resize were taken away by the lock, with the flags to + * give back when it lifts. */ + const dragLockedRegionsRef = useRef< + { region: Region; resize: boolean; drag: boolean }[] + >([]); // setupRegions runs from a once-registered 'ready' handler, so it can only // reach this prop through a ref (like singleRegionRef). const disableDragSelectionRef = useRef(disableDragSelection ?? false); @@ -194,7 +199,33 @@ export function useWaveSurferRegions( }, [applyRegionColor]); 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 { + dragLockedRegionsRef.current.forEach(({ region: r, resize, drag }) => + r.setOptions({ resize, drag }) + ); + dragLockedRegionsRef.current = []; + } + // regions() reads a ref, so it needs no dep of its own. + // eslint-disable-next-line react-hooks/exhaustive-deps }, [lockSegmentSelection]); useEffect(() => { @@ -562,6 +593,7 @@ export function useWaveSurferRegions( regionsPlugin.on( 'region-update', function (r: Region, side?: UpdateSide) { + if (lockSegmentSelectionRef.current) return; resizingRef.current = r.resize; // Live-clamp the boundary as the user drags so regions never visually // overlap: the dragged boundary stops at the neighbor's edge and the @@ -574,6 +606,12 @@ export function useWaveSurferRegions( regionsPlugin.on( 'region-updated', function (r: Region, side?: UpdateSide) { + // Dragging a boundary reports the resized region as the current one, + // which moved the selection mid-record just as a click did — and the + // clamp below would drag the neighbour's shared boundary with it + // (TT-7437). The lock takes drag/resize away, so this is the backstop + // for a gesture already in flight when it went up. + if (lockSegmentSelectionRef.current) return; if (singleRegionRef.current) { if (!loadingRef.current) { waitForIt( @@ -614,8 +652,14 @@ export function useWaveSurferRegions( // Ignore region-in for any region other than the one we're targeting so // the adjacent segment isn't spuriously selected. if (playRegionRef.current && r.id !== playRegionRef.current.id) return; - // lockSegmentSelection does not apply here — playhead-driven updates must - // still flow so playback/overshoot logic works; consumers guard effects. + // The lock applies here too. A click on the waveform is both a click + // and a seek: region-clicked is dropped below, but the seek still walks + // the playhead into the clicked segment and region-in fires behind the + // lock's back. That is how the selection kept moving mid-record even + // with the click blocked (TT-7437). Nothing legitimate is lost — + // recording forces playback off, so there is no playback or overshoot + // to track while the lock is up. + if (lockSegmentSelectionRef.current) return; if (!loopingRef.current) setCurrentRegion(r); }); regionsPlugin.on('region-out', function (r: Region) { From bcf98fc9015c8dfb365ba48344ab7049a80258ea Mon Sep 17 00:00:00 2001 From: Noel Chou Date: Thu, 3 Sep 2026 12:03:04 -0400 Subject: [PATCH 03/27] TT-7437 close three more routes around the recording lock MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit 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) --- .../PassageDetailGuidedPhraseRecord.tsx | 4 ++ .../src/crud/useWavesurferRegions.test.tsx | 56 ++++++++++++++++++- .../src/crud/useWavesurferRegions.tsx | 43 +++++++++----- 3 files changed, 87 insertions(+), 16 deletions(-) diff --git a/src/renderer/src/components/PassageDetail/PassageDetailGuidedPhraseRecord.tsx b/src/renderer/src/components/PassageDetail/PassageDetailGuidedPhraseRecord.tsx index bf5fe105f..e076cfa3b 100644 --- a/src/renderer/src/components/PassageDetail/PassageDetailGuidedPhraseRecord.tsx +++ b/src/renderer/src/components/PassageDetail/PassageDetailGuidedPhraseRecord.tsx @@ -958,6 +958,10 @@ export function PassageDetailGuidedPhraseRecord({ saveRejectedRef.current = false; setSaveRejected(false); pendingOvershootSwallowRef.current = false; + // A latched clause belongs to the mediafile it was recorded against; a new + // source has different clauses, so carrying it over would file the next + // take on a region from the old waveform (TT-7437). + latchRecordingTarget(undefined); optimisticCompletedRef.current.clear(); setHeardIndices([]); setCurrentClausePlayed(false); diff --git a/src/renderer/src/crud/useWavesurferRegions.test.tsx b/src/renderer/src/crud/useWavesurferRegions.test.tsx index ac0af0923..2763f2638 100644 --- a/src/renderer/src/crud/useWavesurferRegions.test.tsx +++ b/src/renderer/src/crud/useWavesurferRegions.test.tsx @@ -51,7 +51,17 @@ jest.mock('wavesurfer.js/dist/plugins/regions', () => { getRegions() { return this.regionList; } - addRegion(r: any) { + addRegion(params: any) { + // wavesurfer hands back a Region, not the params object — the hook + // immediately calls setOptions on it. + const r: any = { + id: `added-${this.regionList.length}`, + attributes: {}, + ...params, + setOptions: jest.fn((o: any) => Object.assign(r, o)), + play: jest.fn(), + remove: jest.fn(), + }; this.regionList.push(r); return r; } @@ -90,9 +100,15 @@ const DURATION = 30; interface IHarnessOpts { lockSegmentSelection: boolean; + /** Playhead position. A split happens at the playhead, so the double-click + * tests need one that is inside a segment rather than on its edge. */ + progressAt?: number; } -const renderRegions = ({ lockSegmentSelection }: IHarnessOpts) => { +const renderRegions = ({ + lockSegmentSelection, + progressAt = 0, +}: IHarnessOpts) => { const plugin = newPlugin(); // three contiguous segments, linked the way setPrevNext links them const segs = [ @@ -130,7 +146,7 @@ const renderRegions = ({ lockSegmentSelection }: IHarnessOpts) => { () => DURATION, () => false, goto, - () => 0, + () => progressAt, () => false, setPlaying, onCurrentRegion, @@ -225,6 +241,22 @@ describe('useWaveSurferRegions — segment selection unlocked', () => { 1 ); }); + + it('double-clicking splits the segment', () => { + const { plugin, segs } = renderRegions({ + lockSegmentSelection: false, + progressAt: 15, + }); + const before = plugin.regionList.length; + + act(() => { + plugin.emit('region-double-clicked', segs[1], { + stopPropagation: jest.fn(), + }); + }); + + expect(plugin.regionList.length).toBeGreaterThan(before); + }); }); describe('useWaveSurferRegions — segment selection locked while recording (TT-7437)', () => { @@ -276,6 +308,24 @@ describe('useWaveSurferRegions — segment selection locked while recording (TT- ); }); + it('does not split a segment on double-click', () => { + // Double-click splits, which reshapes the segment map exactly as a boundary + // drag does — the take's own boundaries would move under it. + const { plugin, segs } = renderRegions({ + lockSegmentSelection: true, + progressAt: 15, + }); + const before = plugin.regionList.length; + + act(() => { + plugin.emit('region-double-clicked', segs[1], { + stopPropagation: jest.fn(), + }); + }); + + expect(plugin.regionList).toHaveLength(before); + }); + it('refuses prev/next segment navigation', () => { const { result } = renderRegions({ lockSegmentSelection: true }); diff --git a/src/renderer/src/crud/useWavesurferRegions.tsx b/src/renderer/src/crud/useWavesurferRegions.tsx index c6a47113c..a833cea6d 100644 --- a/src/renderer/src/crud/useWavesurferRegions.tsx +++ b/src/renderer/src/crud/useWavesurferRegions.tsx @@ -198,6 +198,18 @@ export function useWaveSurferRegions( applyRegionColorRef.current = applyRegionColor; }, [applyRegionColor]); + /** Take a region's drag/resize away for the duration of the lock, remembering + * the flags it had so unlock gives back exactly those. */ + const freezeRegionDrag = (r: Region) => { + if (dragLockedRegionsRef.current.some((e) => e.region === r)) return; + dragLockedRegionsRef.current.push({ + region: r, + resize: r.resize, + drag: r.drag, + }); + r.setOptions({ resize: false, drag: false }); + }; + useEffect(() => { const locked = lockSegmentSelection ?? false; lockSegmentSelectionRef.current = locked; @@ -210,14 +222,7 @@ export function useWaveSurferRegions( // 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 }) - ); + regions().forEach(freezeRegionDrag); } else { dragLockedRegionsRef.current.forEach(({ region: r, resize, drag }) => r.setOptions({ resize, drag }) @@ -353,6 +358,10 @@ export function useWaveSurferRegions( // handle region double-clicks with deduplication // This is an event handler, not a render function, so Date.now() is safe here const handleRegionDoubleClick = (r: Region) => { + // Double-click splits the segment. That reshapes the segment map under a + // take in progress just as a boundary drag would, so it is locked with the + // rest of them (TT-7437). + if (lockSegmentSelectionRef.current) return; const currentTime = getCurrentTime(); const timeSinceLastDoubleClick = currentTime - lastDoubleClickTimeRef.current; @@ -527,6 +536,11 @@ export function useWaveSurferRegions( regionsPlugin.on('region-created', function (r: Region) { if (isMarker(r)) return; r.drag = singleRegionRef.current; + // A region born while the lock is up — the waveform loading with the + // lock already on, or a reload during it — never went through the + // freeze in the lock effect, so it would arrive draggable. Freeze it + // here instead, where every region passes exactly once (TT-7437). + if (lockSegmentSelectionRef.current) freezeRegionDrag(r); // Round region start and end to 5 decimal places because the seek uses 5 decimal places r.start = roundToFiveDecimals(r.start); @@ -606,11 +620,14 @@ export function useWaveSurferRegions( regionsPlugin.on( 'region-updated', function (r: Region, side?: UpdateSide) { - // Dragging a boundary reports the resized region as the current one, - // which moved the selection mid-record just as a click did — and the - // clamp below would drag the neighbour's shared boundary with it - // (TT-7437). The lock takes drag/resize away, so this is the backstop - // for a gesture already in flight when it went up. + // When the user finishes dragging a segment edge, this selects the + // segment they dragged and moves the neighbor's edge to match. Both + // are things a recording in progress must not have happen to it: the + // take belongs to one segment, at the size it was when recording + // started (TT-7437). + // + // While locked, the segments can't be dragged at all, so we only get + // here if the user was already dragging when recording began. if (lockSegmentSelectionRef.current) return; if (singleRegionRef.current) { if (!loadingRef.current) { From d8a298e2a39eb25974ba9e57ebfa49af02ec6b16 Mon Sep 17 00:00:00 2001 From: Noel Chou Date: Thu, 3 Sep 2026 12:55:44 -0400 Subject: [PATCH 04/27] TT-7666 tests: a boundary next to a recording cannot be dragged MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit 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) --- .../src/crud/useWavesurferRegions.test.tsx | 92 ++++++++++++++++++- .../src/crud/useWavesurferRegions.tsx | 12 ++- 2 files changed, 102 insertions(+), 2 deletions(-) diff --git a/src/renderer/src/crud/useWavesurferRegions.test.tsx b/src/renderer/src/crud/useWavesurferRegions.test.tsx index 2763f2638..a14c3390b 100644 --- a/src/renderer/src/crud/useWavesurferRegions.test.tsx +++ b/src/renderer/src/crud/useWavesurferRegions.test.tsx @@ -103,11 +103,15 @@ interface IHarnessOpts { /** Playhead position. A split happens at the playhead, so the double-click * tests need one that is inside a segment rather than on its edge. */ progressAt?: number; + /** Sorted indices of segments that already have a recording. A boundary + * between a recorded segment and its neighbor may not be dragged (TT-7666). */ + recordedIndices?: number[]; } const renderRegions = ({ lockSegmentSelection, progressAt = 0, + recordedIndices = [], }: IHarnessOpts) => { const plugin = newPlugin(); // three contiguous segments, linked the way setPrevNext links them @@ -135,6 +139,9 @@ const renderRegions = ({ const goto = jest.fn(); const onRegion = jest.fn(); const setPlaying = jest.fn(); + const isSegmentRecorded = jest.fn((index: number) => + recordedIndices.includes(index) + ); const { result } = renderHook(() => useWaveSurferRegions( @@ -159,7 +166,8 @@ const renderRegions = ({ lockSegmentSelection, undefined, // getDecodedBuffer true, // disableDragSelection - onRegionClicked + onRegionClicked, + isSegmentRecorded ) ); @@ -175,6 +183,7 @@ const renderRegions = ({ onRegionClicked, goto, setPlaying, + isSegmentRecorded, }; }; @@ -333,3 +342,84 @@ describe('useWaveSurferRegions — segment selection locked while recording (TT- expect(result.current.wsNextRegion()).toBe(false); }); }); + +describe('useWaveSurferRegions — boundary drag on a recorded segment (TT-7666)', () => { + // A recording is tied to a segment's exact time range, so once a segment has + // a Phrase BT (or Careful Speech) recording its boundaries are frozen. A + // boundary is shared by two segments, so a drag is refused when *either* + // side of it is recorded — this is separate from the recording-in-progress + // lock: it holds whenever the neighbouring recording exists, recording or + // not. + + it('refuses to move a recorded segment via its own boundary', () => { + // Segment 1 is recorded. Dragging its end would resize it. + const { plugin, segs, onCurrentRegion } = renderRegions({ + lockSegmentSelection: false, + recordedIndices: [1], + }); + + dragBoundary(plugin, segs[1], 'end', 22); + + // The shared neighbour was not pulled along, and nothing downstream heard a + // boundary change. + expect(segs[2].start).toBe(20); + expect(onCurrentRegion).not.toHaveBeenCalled(); + }); + + it('refuses to move a recorded neighbour via the shared boundary', () => { + // Segment 1 is unrecorded but segment 2 is recorded; dragging segment 1's + // end drags segment 2's start with it, which would reshape the recording. + const { plugin, segs, onCurrentRegion } = renderRegions({ + lockSegmentSelection: false, + recordedIndices: [2], + }); + + dragBoundary(plugin, segs[1], 'end', 22); + + expect(segs[2].start).toBe(20); + expect(onCurrentRegion).not.toHaveBeenCalled(); + }); + + it('refuses a start-side drag that would reshape a recorded neighbour', () => { + // Segment 0 is recorded; segment 1's start is its shared boundary. + const { plugin, segs, onCurrentRegion } = renderRegions({ + lockSegmentSelection: false, + recordedIndices: [0], + }); + + dragBoundary(plugin, segs[1], 'start', 8); + + expect(segs[0].end).toBe(10); + expect(onCurrentRegion).not.toHaveBeenCalled(); + }); + + it('still allows dragging a boundary between two unrecorded segments', () => { + // Segment 2 is recorded, but segment 1's *start* boundary is shared with + // segment 0 — both unrecorded — so it must stay draggable. The lock is + // per-boundary, not "any recording nearby freezes everything". + const { plugin, segs, onCurrentRegion } = renderRegions({ + lockSegmentSelection: false, + recordedIndices: [2], + }); + + dragBoundary(plugin, segs[1], 'start', 8); + + expect(onCurrentRegion).toHaveBeenCalledWith( + expect.objectContaining({ start: 8 }), + 1 + ); + }); + + it('removes the resize handles from a recorded segment', () => { + // The user should not be offered a handle they cannot use — same cue as the + // recording-in-progress lock. + const { segs } = renderRegions({ + lockSegmentSelection: false, + recordedIndices: [1], + }); + + expect(segs[1].setOptions).toHaveBeenCalledWith( + expect.objectContaining({ resize: false }) + ); + }); +}); diff --git a/src/renderer/src/crud/useWavesurferRegions.tsx b/src/renderer/src/crud/useWavesurferRegions.tsx index a833cea6d..0b8f77413 100644 --- a/src/renderer/src/crud/useWavesurferRegions.tsx +++ b/src/renderer/src/crud/useWavesurferRegions.tsx @@ -137,7 +137,15 @@ export function useWaveSurferRegions( * 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 + onRegionClicked?: (region: IRegion) => void, + /** + * Whether the segment at the given sorted index already has a recording. A + * recording is tied to a segment's exact time range, so a boundary shared + * with a recorded segment may not be dragged — regardless of the + * recording-in-progress lock (TT-7666). Keyed on sorted index, like + * applyRegionColor, because that is how consumers track completion. + */ + isSegmentRecorded?: (sortedIndex: number) => boolean ) { const theme = useTheme(); const wsRef = useRef(ws); @@ -169,6 +177,8 @@ export function useWaveSurferRegions( // setupRegions runs from a once-registered 'ready' handler, so it can only // reach this prop through a ref (like singleRegionRef). const disableDragSelectionRef = useRef(disableDragSelection ?? false); + const isSegmentRecordedRef = useRef(isSegmentRecorded); + isSegmentRecordedRef.current = isSegmentRecorded; const regionBeforeClickRef = useRef(undefined); const playTimeoutRef = useRef(undefined); /** Suppress region-in while the playhead is moved programmatically (table row click). */ From f465621daad04007ef6a7da9bd860d4f5245f08b Mon Sep 17 00:00:00 2001 From: Noel Chou Date: Thu, 3 Sep 2026 14:20:26 -0400 Subject: [PATCH 05/27] TT-7666 freeze boundaries and disable +/- on a recorded segment MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit 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) --- .../PassageDetailGuidedPhraseRecord.tsx | 13 +++ .../PassageDetail/PassageDetailPlayer.tsx | 5 + src/renderer/src/components/WSAudioPlayer.tsx | 31 +++++- .../src/components/WSAudioPlayerSegment.tsx | 11 ++- .../WSAudioPlayerSegmentRecordedLock.test.ts | 68 ++++++++++++++ .../src/components/segmentBoundaryLocks.ts | 57 +++++++++++ src/renderer/src/crud/useWaveSurfer.tsx | 7 +- .../src/crud/useWavesurferRegions.test.tsx | 94 +++++++++++++++++++ .../src/crud/useWavesurferRegions.tsx | 81 +++++++++++++++- 9 files changed, 359 insertions(+), 8 deletions(-) create mode 100644 src/renderer/src/components/WSAudioPlayerSegmentRecordedLock.test.ts create mode 100644 src/renderer/src/components/segmentBoundaryLocks.ts diff --git a/src/renderer/src/components/PassageDetail/PassageDetailGuidedPhraseRecord.tsx b/src/renderer/src/components/PassageDetail/PassageDetailGuidedPhraseRecord.tsx index e076cfa3b..272c34f88 100644 --- a/src/renderer/src/components/PassageDetail/PassageDetailGuidedPhraseRecord.tsx +++ b/src/renderer/src/components/PassageDetail/PassageDetailGuidedPhraseRecord.tsx @@ -483,6 +483,18 @@ export function PassageDetailGuidedPhraseRecord({ ] ); + // A segment counts as recorded — its boundaries frozen (TT-7666) — once it + // has a stored take, or a just-saved one rowData has not caught up to yet + // (the same optimistic set the green coloring uses). Only in the recording + // pass: during the listen pass there are no takes, so boundaries stay free. + const isSegmentRecorded = useCallback( + (index: number) => + recordingPassStarted && + (completedIndices.has(index) || + optimisticCompletedRef.current.has(index)), + [recordingPassStarted, completedIndices] + ); + const allClausesComplete = useMemo( () => clauseRegions.length > 0 && completedIndices.size >= clauseRegions.length, @@ -1945,6 +1957,7 @@ export function PassageDetailGuidedPhraseRecord({ onPlayStatusNotify={handlePlayStatusNotify} beforePlay={handleBeforeSourcePlay} lockSegmentSelection={segmentSelectionLocked} + isSegmentRecorded={isSegmentRecorded} allowZoom={true} /> )} diff --git a/src/renderer/src/components/PassageDetail/PassageDetailPlayer.tsx b/src/renderer/src/components/PassageDetail/PassageDetailPlayer.tsx index 2d78dee9e..108687d6f 100644 --- a/src/renderer/src/components/PassageDetail/PassageDetailPlayer.tsx +++ b/src/renderer/src/components/PassageDetail/PassageDetailPlayer.tsx @@ -118,6 +118,9 @@ export interface DetailPlayerProps { beforePlay?: () => void | Promise; /** When true, waveform region clicks cannot change the selected segment. */ lockSegmentSelection?: boolean; + /** Whether the segment at a sorted index already has a recording; freezes its + * boundaries and disables the +/- controls that would reshape it (TT-7666). */ + isSegmentRecorded?: (sortedIndex: number) => boolean; /** Show the "view transcription" button when a transcription exists. Default true. * Set false where the button isn't wanted (e.g. Mark Verses Mobile). */ showTranscriptionButton?: boolean; @@ -130,6 +133,7 @@ export function PassageDetailPlayer(props: DetailPlayerProps) { allowSegment, allowAutoSegment, hideSegmentControls, + isSegmentRecorded, saveSegments, suggestedSegments, forceRegionOnly, @@ -454,6 +458,7 @@ export function PassageDetailPlayer(props: DetailPlayerProps) { regionOnly={requestPlay.regionOnly} forceRegionOnly={forceRegionOnly} lockSegmentSelection={lockSegmentSelection} + isSegmentRecorded={isSegmentRecorded} request={requestPlay.request} loading={loading} busy={pdBusy} diff --git a/src/renderer/src/components/WSAudioPlayer.tsx b/src/renderer/src/components/WSAudioPlayer.tsx index 9c19b2bbe..baa1c1c41 100644 --- a/src/renderer/src/components/WSAudioPlayer.tsx +++ b/src/renderer/src/components/WSAudioPlayer.tsx @@ -70,6 +70,10 @@ import { parseRegions, } from '../crud/useWavesurferRegions'; import WSAudioPlayerSegment from './WSAudioPlayerSegment'; +import { + isAddBlockedByRecording, + isRemoveBlockedByRecording, +} from './segmentBoundaryLocks'; import Confirm from './AlertDialog'; import { getSortedRegions, NamedRegions } from '../utils/namedSegments'; import { @@ -181,6 +185,9 @@ interface IProps { lockSegmentSelection?: boolean; /** When true, disable drag-to-create-region (the red loop region) even in single-region/record mode. */ disableDragSelection?: boolean; + /** Whether the segment at a sorted index already has a recording. Freezes its + * boundaries and disables the +/- controls that would reshape it (TT-7666). */ + isSegmentRecorded?: (sortedIndex: number) => boolean; onMarkerClick?: (time: number) => void; reload?: (blob: Blob) => void; noNewVoice?: boolean; @@ -386,6 +393,7 @@ function WSAudioPlayer(props: IProps) { onSegmentClick, forceRegionOnly, lockSegmentSelection, + isSegmentRecorded, disableDragSelection, onMarkerClick, reload, @@ -770,7 +778,8 @@ function WSAudioPlayer(props: IProps) { applyRegionColor, lockSegmentSelection, disableDragSelection, - onSegmentClick + onSegmentClick, + isSegmentRecorded ); //because we have to call hooks consistently, call this even if we aren't going to record @@ -2089,6 +2098,22 @@ function WSAudioPlayer(props: IProps) { ), [progress, regionBounds] ); + // A recorded segment's boundaries are frozen, so +/- that would reshape it are + // disabled too — not merely inert (TT-7666). + const addBlockedByRecording = useMemo( + () => isAddBlockedByRecording(progress, regionBounds, isSegmentRecorded), + [progress, regionBounds, isSegmentRecorded] + ); + const removeBlockedByRecording = useMemo( + () => + isRemoveBlockedByRecording( + progress, + regionBounds, + SEGMENT_BOUNDARY_TOLERANCE_SEC, + isSegmentRecorded + ), + [progress, regionBounds, isSegmentRecorded] + ); const renderSegmentControls = () => ( ); diff --git a/src/renderer/src/components/WSAudioPlayerSegment.tsx b/src/renderer/src/components/WSAudioPlayerSegment.tsx index dcc00e2cb..2bb0b7d52 100644 --- a/src/renderer/src/components/WSAudioPlayerSegment.tsx +++ b/src/renderer/src/components/WSAudioPlayerSegment.tsx @@ -120,6 +120,11 @@ function WSAudioPlayerSegment(props: IProps) { const handleShowSettings = () => { setShowSettings(!showSettings); }; + // Add is off when it isn't ready, mid-operation, or the playhead sits on a + // boundary / inside a recorded segment (disableSplit carries both). Kept as + // one value so the button's disabled state and its styling can't drift apart. + const splitDisabled = !ready || busyRef.current || !!disableSplit; + const handleSplit = () => { if (!readyRef.current) return false; if (setBusy) setBusy(true); @@ -199,8 +204,10 @@ function WSAudioPlayerSegment(props: IProps) { diff --git a/src/renderer/src/components/WSAudioPlayerSegmentRecordedLock.test.ts b/src/renderer/src/components/WSAudioPlayerSegmentRecordedLock.test.ts new file mode 100644 index 000000000..d23e7df91 --- /dev/null +++ b/src/renderer/src/components/WSAudioPlayerSegmentRecordedLock.test.ts @@ -0,0 +1,68 @@ +import { + isAddBlockedByRecording, + isRemoveBlockedByRecording, +} from './segmentBoundaryLocks'; +import { IRegion } from '../crud/useWavesurferRegions'; + +/** + * The +/- segment controls must be disabled — not merely inert — over a + * recorded segment (TT-7666). These are the pure predicates the player uses to + * decide that; the UI feeds them the playhead position and the segment map. + */ + +// three contiguous 10s segments +const regions: IRegion[] = [ + { start: 0, end: 10 }, + { start: 10, end: 20 }, + { start: 20, end: 30 }, +]; + +const TOL = 0.1; + +describe('isAddBlockedByRecording — Add (+) over a recorded segment', () => { + it('blocks when the playhead is inside a recorded segment', () => { + const recorded = (i: number) => i === 1; + // playhead at 15s is inside segment 1 + expect(isAddBlockedByRecording(15, regions, recorded)).toBe(true); + }); + + it('allows when the playhead is inside an unrecorded segment', () => { + const recorded = (i: number) => i === 1; + // playhead at 5s is inside segment 0 + expect(isAddBlockedByRecording(5, regions, recorded)).toBe(false); + }); + + it('allows when nothing is recorded', () => { + expect(isAddBlockedByRecording(15, regions, () => false)).toBe(false); + }); + + it('allows when no recording predicate is supplied', () => { + expect(isAddBlockedByRecording(15, regions, undefined)).toBe(false); + }); +}); + +describe('isRemoveBlockedByRecording — Remove (-) across a recorded boundary', () => { + it('blocks at the join when the segment before it is recorded', () => { + const recorded = (i: number) => i === 1; + // join between segment 1 and 2 sits at 20s + expect(isRemoveBlockedByRecording(20, regions, TOL, recorded)).toBe(true); + }); + + it('blocks at the join when the segment after it is recorded', () => { + const recorded = (i: number) => i === 2; + // join between segment 1 and 2 at 20s; segment 2 (after) is recorded + expect(isRemoveBlockedByRecording(20, regions, TOL, recorded)).toBe(true); + }); + + it('allows at a join between two unrecorded segments', () => { + const recorded = (i: number) => i === 2; + // join between segment 0 and 1 at 10s; neither is recorded + expect(isRemoveBlockedByRecording(10, regions, TOL, recorded)).toBe(false); + }); + + it('allows when the playhead is not near any join', () => { + const recorded = (i: number) => i === 1; + // 15s is mid-segment, not on a boundary + expect(isRemoveBlockedByRecording(15, regions, TOL, recorded)).toBe(false); + }); +}); diff --git a/src/renderer/src/components/segmentBoundaryLocks.ts b/src/renderer/src/components/segmentBoundaryLocks.ts new file mode 100644 index 000000000..fd7e36554 --- /dev/null +++ b/src/renderer/src/components/segmentBoundaryLocks.ts @@ -0,0 +1,57 @@ +import { IRegion } from '../crud/useWavesurferRegions'; + +/** + * Pure helpers deciding when the player's +/- segment controls must be disabled + * because a recording depends on the boundary (TT-7666). Kept out of + * WSAudioPlayer so they can be unit-tested without the component's app-wide + * import tree. + */ + +/** Sorted index of the segment the playhead sits in, or -1. The last segment + * includes its end so the very end of the track still resolves. */ +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; +} + +/** Add (+) would split the segment under the playhead; block it when that + * segment is recorded (TT-7666). */ +export function isAddBlockedByRecording( + progressSec: number, + regions: IRegion[], + isSegmentRecorded?: (index: number) => boolean +): boolean { + if (!isSegmentRecorded) return false; + const idx = segmentIndexAtProgress(progressSec, regions); + return idx >= 0 && isSegmentRecorded(idx); +} + +/** Remove (−) merges the two segments flanking the internal join near the + * playhead; block it when either is recorded (TT-7666). */ +export function isRemoveBlockedByRecording( + progressSec: number, + regions: IRegion[], + tol: number, + isSegmentRecorded?: (index: number) => boolean +): boolean { + 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); + } + } + return false; +} diff --git a/src/renderer/src/crud/useWaveSurfer.tsx b/src/renderer/src/crud/useWaveSurfer.tsx index 9808dc1e5..7fed701fb 100644 --- a/src/renderer/src/crud/useWaveSurfer.tsx +++ b/src/renderer/src/crud/useWaveSurfer.tsx @@ -64,7 +64,9 @@ export function useWaveSurfer( lockSegmentSelection?: boolean, disableDragSelection?: boolean, /** A region was clicked, as opposed to selected by the playhead. */ - onSegmentClick?: (region: IRegion) => void + onSegmentClick?: (region: IRegion) => void, + /** Whether the segment at a sorted index already has a recording (TT-7666). */ + isSegmentRecorded?: (sortedIndex: number) => boolean ) { const { isMobile } = useMobile(); const [errorReporter] = useGlobal('errorReporter'); @@ -313,7 +315,8 @@ export function useWaveSurfer( lockSegmentSelection, () => blobAudioRef.current, disableDragSelection, - onSegmentClick + onSegmentClick, + isSegmentRecorded ); const setPlayingx = (value: boolean, regionOnly: boolean) => { diff --git a/src/renderer/src/crud/useWavesurferRegions.test.tsx b/src/renderer/src/crud/useWavesurferRegions.test.tsx index a14c3390b..d96fdd9e6 100644 --- a/src/renderer/src/crud/useWavesurferRegions.test.tsx +++ b/src/renderer/src/crud/useWavesurferRegions.test.tsx @@ -423,3 +423,97 @@ describe('useWaveSurferRegions — boundary drag on a recorded segment (TT-7666) ); }); }); + +describe('useWaveSurferRegions — the +/- controls on a recorded segment (TT-7666)', () => { + // The Add (+) button splits the segment under the playhead; Remove (-) merges + // the current segment with a neighbour. Neither may touch a recorded segment, + // and a refused click must be inert — no divider added or removed, and the + // playhead left where it was. + + it('adds a divider inside an unrecorded segment (control)', () => { + const { result, plugin, goto } = renderRegions({ + lockSegmentSelection: false, + progressAt: 15, // inside segment 1 + }); + const before = plugin.regionList.length; + + let ret: unknown; + act(() => { + ret = result.current.wsAddRegion(); + }); + + expect(ret).toBeDefined(); + expect(plugin.regionList.length).toBe(before + 1); + expect(goto).toHaveBeenCalled(); + }); + + it('Add does nothing inside a recorded segment', () => { + const { result, plugin, goto } = renderRegions({ + lockSegmentSelection: false, + progressAt: 15, // inside segment 1, which is recorded + recordedIndices: [1], + }); + const before = plugin.regionList.length; + + let ret: unknown; + act(() => { + ret = result.current.wsAddRegion(); + }); + + expect(ret).toBeUndefined(); + expect(plugin.regionList.length).toBe(before); // no divider added + expect(goto).not.toHaveBeenCalled(); // playhead not moved + }); + + it('Remove merges two unrecorded segments (control)', () => { + const { result, plugin, segs } = renderRegions({ + lockSegmentSelection: false, + progressAt: 10, + }); + playheadEnters(plugin, segs[1]); // current segment = 1 + + let ret: unknown; + act(() => { + ret = result.current.wsRemoveSplitRegion(); + }); + + expect(ret).toBeDefined(); + expect(segs[2].remove).toHaveBeenCalled(); // merged the next segment away + }); + + it('Remove does nothing when the current segment is recorded', () => { + const { result, plugin, segs, goto } = renderRegions({ + lockSegmentSelection: false, + progressAt: 10, + recordedIndices: [1], + }); + playheadEnters(plugin, segs[1]); + goto.mockClear(); + + let ret: unknown; + act(() => { + ret = result.current.wsRemoveSplitRegion(); + }); + + expect(ret).toBeUndefined(); + expect(segs[2].remove).not.toHaveBeenCalled(); // no divider removed + expect(goto).not.toHaveBeenCalled(); // playhead not moved + }); + + it('Remove does nothing when the neighbour it would merge is recorded', () => { + const { result, plugin, segs } = renderRegions({ + lockSegmentSelection: false, + progressAt: 10, + recordedIndices: [2], // the next segment carries the recording + }); + playheadEnters(plugin, segs[1]); + + let ret: unknown; + act(() => { + ret = result.current.wsRemoveSplitRegion(); + }); + + expect(ret).toBeUndefined(); + expect(segs[2].remove).not.toHaveBeenCalled(); + }); +}); diff --git a/src/renderer/src/crud/useWavesurferRegions.tsx b/src/renderer/src/crud/useWavesurferRegions.tsx index 0b8f77413..b23667100 100644 --- a/src/renderer/src/crud/useWavesurferRegions.tsx +++ b/src/renderer/src/crud/useWavesurferRegions.tsx @@ -284,6 +284,53 @@ export function useWaveSurferRegions( r.setOptions({ color: base }); } }); + applyRecordedResizeLocks(); + }; + + /** + * Take the resize handles off any boundary a recording depends on (TT-7666). + * A recording is tied to a segment's exact time range, so once a segment is + * recorded its edges are frozen — and a boundary is shared by two segments, + * so both regions touching it lose the handle on that side. Recomputed on + * every color pass (the same trigger as green/pending), so deleting a take + * gives the handles straight back. Idempotent: each region's full desired + * state is set every time, so it self-corrects as recordings come and go. + */ + 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), + }); + } + }); + }; + + /** + * Whether the boundary the user is dragging on region `r` (its `side` edge, + * or either edge when the side is unknown) is shared with a recorded segment + * — in which case the drag must be refused (TT-7666). + */ + const isRecordedBoundary = (r: Region, side?: UpdateSide) => { + const recorded = isSegmentRecordedRef.current; + if (!recorded) return false; + const idx = regionIndexInSorted(r); + if (idx < 0) return false; + if (recorded(idx)) return true; + // 'start' shares with the previous segment, 'end' with the next; an + // unknown side means check both. + if (side !== 'end' && recorded(idx - 1)) return true; + if (side !== 'start' && recorded(idx + 1)) return true; + return false; }; const Regions = () => regionsRef.current; @@ -618,6 +665,9 @@ export function useWaveSurferRegions( 'region-update', function (r: Region, side?: UpdateSide) { if (lockSegmentSelectionRef.current) return; + // A recorded segment's boundary is frozen; the handle is gone, so + // this is the backstop for a gesture already in flight (TT-7666). + if (isRecordedBoundary(r, side)) return; resizingRef.current = r.resize; // Live-clamp the boundary as the user drags so regions never visually // overlap: the dragged boundary stops at the neighbor's edge and the @@ -639,6 +689,9 @@ export function useWaveSurferRegions( // While locked, the segments can't be dragged at all, so we only get // here if the user was already dragging when recording began. if (lockSegmentSelectionRef.current) return; + // Same for a boundary a recording depends on (TT-7666): its handle is + // gone, so reaching here means a drag was already under way. + if (isRecordedBoundary(r, side)) return; if (singleRegionRef.current) { if (!loadingRef.current) { waitForIt( @@ -756,6 +809,8 @@ export function useWaveSurferRegions( color: 'rgba(255, 0, 0, 0.1)', }); } + // Freeze the handles of any already-recorded segment (TT-7666). + applyRecordedResizeLocks(); } }; @@ -1045,6 +1100,8 @@ export function useWaveSurferRegions( region.id = r?.id; }); setPrevNext(regarray.map((r: any) => r.id)); + // Freeze the handles of recorded segments in the freshly loaded map (TT-7666). + applyRecordedResizeLocks(); onRegion(regarray.length, newRegions); onRegionGoTo(regarray[defaultRegionIndex]?.start ?? 0); loadingRef.current = false; @@ -1146,6 +1203,22 @@ export function useWaveSurferRegions( clearRegions(); return; } + // Removing a boundary merges two segments; refuse if either is recorded — + // it would reshape the recording's segment (TT-7666). Same neighbour choice + // as the merge below (playhead near the start merges with prev, else next). + const recorded = isSegmentRecordedRef.current; + if (recorded) { + const mergePrev = findPrevRegion(r); + const other = + isNear(r.start) && mergePrev ? mergePrev : findNextRegion(r, false); + const otherIdx = other ? regionIndexInSorted(other) : -1; + if ( + recorded(regionIndexInSorted(r)) || + (otherIdx >= 0 && recorded(otherIdx)) + ) { + return undefined; + } + } const ret: IRegionChange = { start: r.start, end: r.end, @@ -1193,7 +1266,13 @@ export function useWaveSurferRegions( }; const wsAddRegion = () => { - return wsSplitRegion(findRegion(progress(), true), progress()); + const target = findRegion(progress(), true); + // No new boundary inside a recorded segment — that would split its audio in + // two (TT-7666). Return without splitting or moving the playhead. + if (target && isSegmentRecordedRef.current?.(regionIndexInSorted(target))) { + return undefined; + } + return wsSplitRegion(target, progress()); }; const wsRemoveCurrentRegion = () => { From 73302edd6c55171aa51d4057186447010f9783f3 Mon Sep 17 00:00:00 2001 From: Noel Chou Date: Thu, 3 Sep 2026 14:35:01 -0400 Subject: [PATCH 06/27] TT-7666 keep recorded boundaries frozen across the recording lock release MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit 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) --- .../src/crud/useWavesurferRegions.test.tsx | 28 ++++++++++++++++++- .../src/crud/useWavesurferRegions.tsx | 27 ++++++++++++++---- 2 files changed, 48 insertions(+), 7 deletions(-) diff --git a/src/renderer/src/crud/useWavesurferRegions.test.tsx b/src/renderer/src/crud/useWavesurferRegions.test.tsx index d96fdd9e6..42b994c2f 100644 --- a/src/renderer/src/crud/useWavesurferRegions.test.tsx +++ b/src/renderer/src/crud/useWavesurferRegions.test.tsx @@ -106,12 +106,16 @@ interface IHarnessOpts { /** Sorted indices of segments that already have a recording. A boundary * between a recorded segment and its neighbor may not be dragged (TT-7666). */ recordedIndices?: number[]; + /** Supply a color function so `applyRegionColors` runs its body (it is a + * no-op without one) — needed to exercise the recorded-resize pass. */ + withColors?: boolean; } const renderRegions = ({ lockSegmentSelection, progressAt = 0, recordedIndices = [], + withColors = false, }: IHarnessOpts) => { const plugin = newPlugin(); // three contiguous segments, linked the way setPrevNext links them @@ -162,7 +166,7 @@ const renderRegions = ({ undefined, // onMarkerClick undefined, // verses undefined, // hasSegmentUndo - undefined, // applyRegionColor + withColors ? () => 'rgba(1, 2, 3, 0.5)' : undefined, // applyRegionColor lockSegmentSelection, undefined, // getDecodedBuffer true, // disableDragSelection @@ -422,6 +426,28 @@ describe('useWaveSurferRegions — boundary drag on a recorded segment (TT-7666) expect.objectContaining({ resize: false }) ); }); + + it('does not re-enable handles on a color pass while recording is locked', () => { + // The recording-in-progress lock (TT-7437) takes every region's handles + // away. The recorded-resize pass runs on every color update and must not + // hand a handle back to an unrecorded segment while that lock holds, or the + // user could resize mid-record. + const { result, segs } = renderRegions({ + lockSegmentSelection: true, + recordedIndices: [1], + withColors: true, + }); + segs.forEach((s) => s.setOptions.mockClear()); + + act(() => { + result.current.applyRegionColors(); + }); + + // segment 0 is unrecorded, but the lock is up: it must not be re-enabled. + expect(segs[0].setOptions).not.toHaveBeenCalledWith( + expect.objectContaining({ resize: true }) + ); + }); }); describe('useWaveSurferRegions — the +/- controls on a recorded segment (TT-7666)', () => { diff --git a/src/renderer/src/crud/useWavesurferRegions.tsx b/src/renderer/src/crud/useWavesurferRegions.tsx index b23667100..a14cb0a4b 100644 --- a/src/renderer/src/crud/useWavesurferRegions.tsx +++ b/src/renderer/src/crud/useWavesurferRegions.tsx @@ -238,6 +238,10 @@ export function useWaveSurferRegions( r.setOptions({ resize, drag }) ); dragLockedRegionsRef.current = []; + // The snapshot restores the flags as they were at lock time; now re-derive + // the recorded/unrecorded resting state, since a take may have been made + // during the lock (TT-7666). No-op while locked, so it is safe here only. + applyRecordedResizeLocks(); } // regions() reads a ref, so it needs no dep of its own. // eslint-disable-next-line react-hooks/exhaustive-deps @@ -299,17 +303,25 @@ export function useWaveSurferRegions( const applyRecordedResizeLocks = () => { const recorded = isSegmentRecordedRef.current; if (!recorded) return; + // While the recording-in-progress lock holds it owns every region's resize + // flag (all off); re-enabling any here would hand back a handle the lock + // took away. The lock's release path re-runs this to restore the resting + // recorded/unrecorded state (TT-7437 / TT-7666). + if (lockSegmentSelectionRef.current) return; const sorted = sortedRegions(); + const last = sorted.length - 1; 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. + // Draggable, except a side shared with a recorded neighbour. The outer + // edges (first start, last end) have no neighbour, so stay free — and + // the predicate is never asked about an out-of-range index. r.setOptions({ resize: true, - resizeStart: !recorded(i - 1), - resizeEnd: !recorded(i + 1), + resizeStart: i === 0 ? true : !recorded(i - 1), + resizeEnd: i === last ? true : !recorded(i + 1), }); } }); @@ -327,9 +339,12 @@ export function useWaveSurferRegions( if (idx < 0) return false; if (recorded(idx)) return true; // 'start' shares with the previous segment, 'end' with the next; an - // unknown side means check both. - if (side !== 'end' && recorded(idx - 1)) return true; - if (side !== 'start' && recorded(idx + 1)) return true; + // unknown side means check both. Guard the ends so the predicate is only + // ever asked about a real sorted index. + if (side !== 'end' && idx > 0 && recorded(idx - 1)) return true; + if (side !== 'start' && idx < numRegions() - 1 && recorded(idx + 1)) { + return true; + } return false; }; From 8e42f09895c0ab99e462bb1235c857767c8543c3 Mon Sep 17 00:00:00 2001 From: Noel Chou Date: Thu, 3 Sep 2026 14:56:50 -0400 Subject: [PATCH 07/27] TT-7666 keep a frozen boundary visible, just not draggable MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit 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) --- .../src/crud/useWavesurferRegions.test.tsx | 31 +++++++-- .../src/crud/useWavesurferRegions.tsx | 63 ++++++++++++------- 2 files changed, 67 insertions(+), 27 deletions(-) diff --git a/src/renderer/src/crud/useWavesurferRegions.test.tsx b/src/renderer/src/crud/useWavesurferRegions.test.tsx index 42b994c2f..157b35b8b 100644 --- a/src/renderer/src/crud/useWavesurferRegions.test.tsx +++ b/src/renderer/src/crud/useWavesurferRegions.test.tsx @@ -414,16 +414,39 @@ describe('useWaveSurferRegions — boundary drag on a recorded segment (TT-7666) ); }); - it('removes the resize handles from a recorded segment', () => { - // The user should not be offered a handle they cannot use — same cue as the - // recording-in-progress lock. + it('freezes a recorded segment but keeps its boundary handles visible', () => { + // The handle is the only clear marker of where a segment ends, so it stays + // rendered (resize: true) — but both edges are turned off so it cannot be + // dragged (TT-7666). const { segs } = renderRegions({ lockSegmentSelection: false, recordedIndices: [1], }); expect(segs[1].setOptions).toHaveBeenCalledWith( - expect.objectContaining({ resize: false }) + expect.objectContaining({ + resize: true, + resizeStart: false, + resizeEnd: false, + }) + ); + }); + + it('freezes only the shared side of an unrecorded neighbour', () => { + // Segment 1 is recorded; its neighbours keep their far edge draggable but + // freeze the edge shared with it. + const { segs } = renderRegions({ + lockSegmentSelection: false, + recordedIndices: [1], + }); + + // segment 0: start is the track edge (free), end is shared with 1 (frozen) + expect(segs[0].setOptions).toHaveBeenCalledWith( + expect.objectContaining({ resizeStart: true, resizeEnd: false }) + ); + // segment 2: start shared with 1 (frozen), end is the track edge (free) + expect(segs[2].setOptions).toHaveBeenCalledWith( + expect.objectContaining({ resizeStart: false, resizeEnd: true }) ); }); diff --git a/src/renderer/src/crud/useWavesurferRegions.tsx b/src/renderer/src/crud/useWavesurferRegions.tsx index a14cb0a4b..7697f81bd 100644 --- a/src/renderer/src/crud/useWavesurferRegions.tsx +++ b/src/renderer/src/crud/useWavesurferRegions.tsx @@ -292,38 +292,55 @@ export function useWaveSurferRegions( }; /** - * Take the resize handles off any boundary a recording depends on (TT-7666). - * A recording is tied to a segment's exact time range, so once a segment is - * recorded its edges are frozen — and a boundary is shared by two segments, - * so both regions touching it lose the handle on that side. Recomputed on - * every color pass (the same trigger as green/pending), so deleting a take - * gives the handles straight back. Idempotent: each region's full desired - * state is set every time, so it self-corrects as recordings come and go. + * The ew-resize cursor wavesurfer puts on a resize handle. Overridden to the + * default cursor on a frozen boundary so it doesn't invite a drag it refuses. + */ + const setHandleCursor = ( + r: Region, + side: 'left' | 'right', + active: boolean + ) => { + const handle = r.element?.querySelector( + `[part*="region-handle-${side}"]` + ) as HTMLElement | null; + if (handle) handle.style.cursor = active ? 'ew-resize' : 'default'; + }; + + /** + * Freeze any boundary a recording depends on (TT-7666). A recording is tied to + * a segment's exact time range, so once a segment is recorded its edges can't + * move — and a boundary is shared by two segments, so both sides of it are + * frozen. The handle stays *visible* (it is the only clear marker of where a + * segment ends), but its resize is turned off and its cursor reverts to the + * default so it does not look draggable. Recomputed on every color pass (the + * same trigger as green/pending), so deleting a take unfreezes it. Idempotent: + * each region's full desired state is set every time. */ const applyRecordedResizeLocks = () => { const recorded = isSegmentRecordedRef.current; if (!recorded) return; // While the recording-in-progress lock holds it owns every region's resize - // flag (all off); re-enabling any here would hand back a handle the lock - // took away. The lock's release path re-runs this to restore the resting - // recorded/unrecorded state (TT-7437 / TT-7666). + // flag; re-enabling any here would fight it. The lock's release path re-runs + // this to restore the resting recorded/unrecorded state (TT-7437 / TT-7666). if (lockSegmentSelectionRef.current) return; const sorted = sortedRegions(); const last = sorted.length - 1; 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. The outer - // edges (first start, last end) have no neighbour, so stay free — and - // the predicate is never asked about an out-of-range index. - r.setOptions({ - resize: true, - resizeStart: i === 0 ? true : !recorded(i - 1), - resizeEnd: i === last ? true : !recorded(i + 1), - }); - } + // A side is draggable only when neither the segment nor the neighbour + // across that side is recorded. The outer edges (first start, last end) + // have no neighbour, so the predicate is never asked an out-of-range index. + const self = recorded(i); + const startActive = !self && (i === 0 || !recorded(i - 1)); + const endActive = !self && (i === last || !recorded(i + 1)); + // resize stays true so the handles (and thus the boundary lines) render; + // resizeStart/resizeEnd gate the actual drag per side. + r.setOptions({ + resize: true, + resizeStart: startActive, + resizeEnd: endActive, + }); + setHandleCursor(r, 'left', startActive); + setHandleCursor(r, 'right', endActive); }); }; From a13cd9f891ad9a6418a6f4df794e1667ccd18743 Mon Sep 17 00:00:00 2001 From: Noel Chou Date: Thu, 3 Sep 2026 16:45:47 -0400 Subject: [PATCH 08/27] TT-7666 docs: explain the kept persistence-layer backstop 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) --- .../PassageDetailGuidedPhraseRecord.tsx | 18 ++++++++++++++++++ 1 file changed, 18 insertions(+) diff --git a/src/renderer/src/components/PassageDetail/PassageDetailGuidedPhraseRecord.tsx b/src/renderer/src/components/PassageDetail/PassageDetailGuidedPhraseRecord.tsx index 272c34f88..c8e0cfe2d 100644 --- a/src/renderer/src/components/PassageDetail/PassageDetailGuidedPhraseRecord.tsx +++ b/src/renderer/src/components/PassageDetail/PassageDetailGuidedPhraseRecord.tsx @@ -1137,6 +1137,24 @@ export function PassageDetailGuidedPhraseRecord({ if (recordingActiveRef.current || savingRecording) return; const regions = getSortedRegions(seg); if (regions.length === 0) return; + // Backstop, not the primary guard. This reverts a segment-map change that + // would move a recorded segment's exact bounds. It was the original + // protection for TT-7666, but in the app a boundary *drag* on a recorded + // segment stuck anyway — the revert did not fire for that path and the + // root cause was never pinned down (by code reading a drag reaches here + // and should revert; it did not). Rather than chase that, TT-7666 blocks + // the offending gestures at their source in useWavesurferRegions: + // - recorded boundaries render but are frozen (resizeStart/resizeEnd off) + // with a default cursor, and region-update/region-updated refuse a + // drag touching a recorded segment; + // - wsAddRegion / wsRemoveSplitRegion refuse to split inside or merge + // across a recorded segment; + // - the +/- buttons are disabled over a recorded boundary. + // Those never produce a violating change, so this line no longer fires for + // drag or +/-. It is kept as defense-in-depth for any *other* writer that + // still reaches handleSegment during the recording pass (resegmentation, + // future paths); it is cheap and harmless when it does not trigger. Remove + // it only once every such path is proven blocked at the source. if ( recordingPassStarted && !preservesRecordedBoundaries(clauseRegions, regions, completedIndices) From 7689629131ddf87515bad40dddd28ed587b59117 Mon Sep 17 00:00:00 2001 From: Noel Chou Date: Thu, 3 Sep 2026 17:03:38 -0400 Subject: [PATCH 09/27] reword comment --- .../PassageDetailGuidedPhraseRecord.tsx | 37 ++++++++++--------- 1 file changed, 19 insertions(+), 18 deletions(-) diff --git a/src/renderer/src/components/PassageDetail/PassageDetailGuidedPhraseRecord.tsx b/src/renderer/src/components/PassageDetail/PassageDetailGuidedPhraseRecord.tsx index c8e0cfe2d..f2ae06b81 100644 --- a/src/renderer/src/components/PassageDetail/PassageDetailGuidedPhraseRecord.tsx +++ b/src/renderer/src/components/PassageDetail/PassageDetailGuidedPhraseRecord.tsx @@ -1137,24 +1137,25 @@ export function PassageDetailGuidedPhraseRecord({ if (recordingActiveRef.current || savingRecording) return; const regions = getSortedRegions(seg); if (regions.length === 0) return; - // Backstop, not the primary guard. This reverts a segment-map change that - // would move a recorded segment's exact bounds. It was the original - // protection for TT-7666, but in the app a boundary *drag* on a recorded - // segment stuck anyway — the revert did not fire for that path and the - // root cause was never pinned down (by code reading a drag reaches here - // and should revert; it did not). Rather than chase that, TT-7666 blocks - // the offending gestures at their source in useWavesurferRegions: - // - recorded boundaries render but are frozen (resizeStart/resizeEnd off) - // with a default cursor, and region-update/region-updated refuse a - // drag touching a recorded segment; - // - wsAddRegion / wsRemoveSplitRegion refuse to split inside or merge - // across a recorded segment; - // - the +/- buttons are disabled over a recorded boundary. - // Those never produce a violating change, so this line no longer fires for - // drag or +/-. It is kept as defense-in-depth for any *other* writer that - // still reaches handleSegment during the recording pass (resegmentation, - // future paths); it is cheap and harmless when it does not trigger. Remove - // it only once every such path is proven blocked at the source. + // Safety backstop, not the main guard. If a segment update would move the + // exact boundaries of a recorded segment, this code reverts that update. + // + // Future work to harden: + // In TT-7666 we discovered this was not reverting segment boundary drags, but + // never finished investigating why not. + // + // Instead we now block those edits where they start, in + // useWavesurferRegions: + // - recorded boundaries are shown but locked (no resize handles) and + // drag events that touch recorded segments are rejected; + // - wsAddRegion / wsRemoveSplitRegion reject splitting inside or + // merging across a recorded segment; + // - the +/- buttons are disabled on recorded boundaries. + // + // Because those paths are blocked earlier, this check usually does not + // fire for drag or +/-. Keep it as defense-in-depth for any other path + // that might still call handleSegment during recording (for example, + // resegmentation or future code changes). if ( recordingPassStarted && !preservesRecordedBoundaries(clauseRegions, regions, completedIndices) From fc608d0acd2c08fc4fc225042823da4c872affd9 Mon Sep 17 00:00:00 2001 From: Noel Chou Date: Thu, 3 Sep 2026 17:59:44 -0400 Subject: [PATCH 10/27] docs(ADR-0011): note the TT-7437 recordingTarget latch as a fifth workaround MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit 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) --- docs/adr/0011-segment-selection-intent.md | 31 +++++++++++++++++++++++ 1 file changed, 31 insertions(+) diff --git a/docs/adr/0011-segment-selection-intent.md b/docs/adr/0011-segment-selection-intent.md index 59dc99f38..58e0f723e 100644 --- a/docs/adr/0011-segment-selection-intent.md +++ b/docs/adr/0011-segment-selection-intent.md @@ -155,3 +155,34 @@ adds, and it removes the failure mode that has produced the most defects. Piece 2 touches the shared engine and affects every consumer of the waveform (Mark Verses, Transcribe, Discuss), so it deserves its own change and its own regression pass over prev/next segment playback. + +## Update — TT-7437 added a fifth workaround (2026-09-03) + +TT-7437 ("recording saved to the currently selected segment instead of the one +played") was fixed without the source-tag refactor above. Two additions, on PR +for TT-7437 / TT-7666: + +- **`recordingTarget` latch** in `PassageDetailGuidedPhraseRecord.tsx` — the + clause `{index, region}` is captured when recording starts and everything that + files the take (`sourceSegments`, filename postfix, the optimistic green mark) + reads the latch instead of the live `currentSegment`. Cleared on + save-complete, discard, and mediafile change. +- **the selection lock extended to `region-in`** in `useWaveSurferRegions` — a + waveform click also seeks, so the playhead entered the clicked region and + `region-in` moved the selection behind the click-lock's back; the lock now + covers that path (and drag/resize, double-click). + +This is aligned with the ADR's thesis — the latch captures **intent explicitly** +(the user pressed Record on _this_ segment) rather than inferring it from the +channel, which is exactly "intent is what is missing." But it is a **fifth** +entry in the "What guessing costs today" table rather than a consolidation: it +does not remove `pendingOvershootSwallowRef`, `suppressClauseAutoPlayRef`, +`currentSegmentSeq`, or `onSegmentClick`, and it adds one more piece of state a +future reader has to hold. + +**When Piece 1 (source-tagged writes) lands, revisit the latch.** With a +`'click'`-only navigation effect the take can no longer be re-filed by a +playhead-driven change mid-record, so the latch may become redundant — or it may +be kept as a deliberate save-time invariant ("a take belongs to the segment it +started on") that is cheaper to prove than to re-derive. Decide it consciously +then rather than leaving a fifth workaround in place by inertia. From c984a1651aaaf217fa15ee6549dc3d3ec555fef0 Mon Sep 17 00:00:00 2001 From: Noel Chou Date: Thu, 3 Sep 2026 18:05:57 -0400 Subject: [PATCH 11/27] chatgpt comment rewrites --- .../PassageDetailCarefulSpeech.test.tsx | 9 +- .../PassageDetailGuidedPhraseRecord.tsx | 72 ++++------- .../PassageDetail/PassageDetailPlayer.tsx | 3 +- src/renderer/src/components/WSAudioPlayer.tsx | 4 +- .../src/components/WSAudioPlayerSegment.tsx | 8 +- .../WSAudioPlayerSegmentRecordedLock.test.ts | 18 ++- .../src/components/segmentBoundaryLocks.ts | 15 +-- .../src/crud/useWavesurferRegions.test.tsx | 83 ++++--------- .../src/crud/useWavesurferRegions.tsx | 114 +++++------------- 9 files changed, 102 insertions(+), 224 deletions(-) diff --git a/src/renderer/src/components/PassageDetail/PassageDetailCarefulSpeech.test.tsx b/src/renderer/src/components/PassageDetail/PassageDetailCarefulSpeech.test.tsx index 9efa10424..82640d3a8 100644 --- a/src/renderer/src/components/PassageDetail/PassageDetailCarefulSpeech.test.tsx +++ b/src/renderer/src/components/PassageDetail/PassageDetailCarefulSpeech.test.tsx @@ -438,8 +438,7 @@ describe('PassageDetailCarefulSpeech — recording segment lock (TT-7437)', () = }); describe('PassageDetailCarefulSpeech — take belongs to the clause it started on (TT-7437)', () => { - // Record a take on clause 2 and hand back the sourceSegments value the - // recorder must file it under. + // Start recording on clause 2 and return the resulting sourceSegments value. const startTakeOnClause2 = async () => { mockCompleted = new Set([0, 1]); // auto-play clause 2 const utils = await mountAndSettle(); @@ -462,10 +461,8 @@ describe('PassageDetailCarefulSpeech — take belongs to the clause it started o }); it('keeps the take on its clause when the selection change lands after stop', async () => { - // The waveform click is dropped while locked, but the seek it caused makes - // the playhead enter the tapped clause; that region-in can arrive in the - // gap between the recorder stopping and the save starting. The take was - // already made — it still belongs to clause 2 (TT-7437). + // Even if the tap seek causes a later region-in, the take still belongs to + // clause 2 because recording already happened there (TT-7437). const { rerender } = await startTakeOnClause2(); await act(async () => { diff --git a/src/renderer/src/components/PassageDetail/PassageDetailGuidedPhraseRecord.tsx b/src/renderer/src/components/PassageDetail/PassageDetailGuidedPhraseRecord.tsx index f2ae06b81..0b010ef03 100644 --- a/src/renderer/src/components/PassageDetail/PassageDetailGuidedPhraseRecord.tsx +++ b/src/renderer/src/components/PassageDetail/PassageDetailGuidedPhraseRecord.tsx @@ -262,15 +262,11 @@ export function PassageDetailGuidedPhraseRecord({ const optimisticCompletedRef = useRef>(new Set()); const currentIndexRef = useRef(0); /** - * The clause a take was started on, latched when capture begins and held - * until the take is stored or discarded (TT-7437). + * Clause and region for the active take (TT-7437). * - * Everything that files a take — sourceSegments, the filename postfix, the - * green completion mark — used to read the *live* selection at save time, so - * any path that moved the selection between Record and the upload misfiled - * the audio. The engine-side lock stops the known routes, but a take belongs - * to the clause it was recorded on whatever slips through, so that clause is - * captured once and read back from here. + * We lock this when recording starts and keep it until the take is saved or + * discarded. Save-time values (sourceSegments, filename postfix, completion + * color) must come from this latched target, not from live selection. */ const [recordingTarget, setRecordingTarget] = useState< { index: number; region: IRegion } | undefined @@ -483,10 +479,9 @@ export function PassageDetailGuidedPhraseRecord({ ] ); - // A segment counts as recorded — its boundaries frozen (TT-7666) — once it - // has a stored take, or a just-saved one rowData has not caught up to yet - // (the same optimistic set the green coloring uses). Only in the recording - // pass: during the listen pass there are no takes, so boundaries stay free. + // A segment is treated as recorded (boundary locked, TT-7666) when it has a + // saved take, or a newly saved take that rowData has not shown yet. + // This only applies during the recording pass. const isSegmentRecorded = useCallback( (index: number) => recordingPassStarted && @@ -720,10 +715,9 @@ export function PassageDetailGuidedPhraseRecord({ // Keyed on the index rather than the navigation handlers because every clause // move funnels through it. useEffect(() => { - // Navigating away from a failed take abandons it, so it gives up its - // latched clause with the message (TT-7437). Only that case: a take that - // is recording, uploading, or waiting to upload keeps its clause however - // the selection moves — that is the whole point of the latch. + // The latch keeps a take tied to its original clause (TT-7437). + // If upload failed and the user navigates away, that take is abandoned, + // so clear the latch here. if (saveRejectedRef.current) latchRecordingTarget(undefined); saveRejectedRef.current = false; setSaveRejected(false); @@ -970,9 +964,8 @@ export function PassageDetailGuidedPhraseRecord({ saveRejectedRef.current = false; setSaveRejected(false); pendingOvershootSwallowRef.current = false; - // A latched clause belongs to the mediafile it was recorded against; a new - // source has different clauses, so carrying it over would file the next - // take on a region from the old waveform (TT-7437). + // The latch is mediafile-specific. Clear it when source media changes, + // so the next take cannot reuse a clause from the old waveform (TT-7437). latchRecordingTarget(undefined); optimisticCompletedRef.current.clear(); setHeardIndices([]); @@ -1137,25 +1130,9 @@ export function PassageDetailGuidedPhraseRecord({ if (recordingActiveRef.current || savingRecording) return; const regions = getSortedRegions(seg); if (regions.length === 0) return; - // Safety backstop, not the main guard. If a segment update would move the - // exact boundaries of a recorded segment, this code reverts that update. - // - // Future work to harden: - // In TT-7666 we discovered this was not reverting segment boundary drags, but - // never finished investigating why not. - // - // Instead we now block those edits where they start, in - // useWavesurferRegions: - // - recorded boundaries are shown but locked (no resize handles) and - // drag events that touch recorded segments are rejected; - // - wsAddRegion / wsRemoveSplitRegion reject splitting inside or - // merging across a recorded segment; - // - the +/- buttons are disabled on recorded boundaries. - // - // Because those paths are blocked earlier, this check usually does not - // fire for drag or +/-. Keep it as defense-in-depth for any other path - // that might still call handleSegment during recording (for example, - // resegmentation or future code changes). + // Defense-in-depth only: if an update still changes recorded boundaries, + // reload the previous regions. Main blocking now happens earlier in + // useWavesurferRegions (drag, split/merge, and +/- controls; TT-7666). if ( recordingPassStarted && !preservesRecordedBoundaries(clauseRegions, regions, completedIndices) @@ -1784,9 +1761,8 @@ export function PassageDetailGuidedPhraseRecord({ // (TT-7552). Only on a real upload though — a terminal failure still calls // us, with no mediaId, and painting that green tells the user their take // was stored when it was not (TT-7583). - // The clause the take was recorded on, not wherever the selection has - // since ended up — the green mark has to land where the audio went - // (TT-7437). + // Mark completion on the clause where recording started (TT-7437), + // not on the current live selection. const takenIndex = recordingTargetRef.current?.index ?? currentIndexRef.current; if (mediaId) { @@ -1795,8 +1771,8 @@ export function PassageDetailGuidedPhraseRecord({ latchRecordingTarget(undefined); } else { optimisticCompletedRef.current.delete(takenIndex); - // Keep the latch on a failed upload: Retry must file the take on the - // same clause, however far the user has wandered (TT-7583). + // Keep the latch on failed upload so Retry files to the same clause + // even if selection moved (TT-7583). } // Stays 'recorded' either way: the take still exists, it just is not // stored. That keeps Record disabled and the clear button available, so @@ -2037,10 +2013,8 @@ export function PassageDetailGuidedPhraseRecord({ onRecording={(active) => { if (active) { recordingActiveRef.current = true; - // Latch the clause this take belongs to. Everything that files - // the take reads it from here, so the audio lands where it was - // recorded no matter what moves the selection afterwards - // (TT-7437). + // Latch the take target at record start so later selection + // changes do not move where this take is filed (TT-7437). if (currentRegion) { latchRecordingTarget({ index: currentIndex, @@ -2078,8 +2052,8 @@ export function PassageDetailGuidedPhraseRecord({ setSavingRecording(false); // Upload failures route through afterUploadCb('') as well, but // MediaRecord's save-requested-with-no-audio branch only lands here, - // so undo the optimistic green from this path too (TT-7583). The - // latch stays up so a Retry still files the take on its own clause. + // Also clear optimistic green on this failure path (TT-7583). + // Keep the latch so Retry still files to the same clause. optimisticCompletedRef.current.delete( recordingTargetRef.current?.index ?? currentIndexRef.current ); diff --git a/src/renderer/src/components/PassageDetail/PassageDetailPlayer.tsx b/src/renderer/src/components/PassageDetail/PassageDetailPlayer.tsx index 108687d6f..fafe6b69a 100644 --- a/src/renderer/src/components/PassageDetail/PassageDetailPlayer.tsx +++ b/src/renderer/src/components/PassageDetail/PassageDetailPlayer.tsx @@ -118,8 +118,7 @@ export interface DetailPlayerProps { beforePlay?: () => void | Promise; /** When true, waveform region clicks cannot change the selected segment. */ lockSegmentSelection?: boolean; - /** Whether the segment at a sorted index already has a recording; freezes its - * boundaries and disables the +/- controls that would reshape it (TT-7666). */ + /** Whether a sorted segment already has a recording (TT-7666). */ isSegmentRecorded?: (sortedIndex: number) => boolean; /** Show the "view transcription" button when a transcription exists. Default true. * Set false where the button isn't wanted (e.g. Mark Verses Mobile). */ diff --git a/src/renderer/src/components/WSAudioPlayer.tsx b/src/renderer/src/components/WSAudioPlayer.tsx index baa1c1c41..22bfa7449 100644 --- a/src/renderer/src/components/WSAudioPlayer.tsx +++ b/src/renderer/src/components/WSAudioPlayer.tsx @@ -2098,8 +2098,8 @@ function WSAudioPlayer(props: IProps) { ), [progress, regionBounds] ); - // A recorded segment's boundaries are frozen, so +/- that would reshape it are - // disabled too — not merely inert (TT-7666). + // If a segment is recorded, disable +/- when they would change its boundary + // (TT-7666). const addBlockedByRecording = useMemo( () => isAddBlockedByRecording(progress, regionBounds, isSegmentRecorded), [progress, regionBounds, isSegmentRecorded] diff --git a/src/renderer/src/components/WSAudioPlayerSegment.tsx b/src/renderer/src/components/WSAudioPlayerSegment.tsx index 2bb0b7d52..685c7088a 100644 --- a/src/renderer/src/components/WSAudioPlayerSegment.tsx +++ b/src/renderer/src/components/WSAudioPlayerSegment.tsx @@ -120,9 +120,8 @@ function WSAudioPlayerSegment(props: IProps) { const handleShowSettings = () => { setShowSettings(!showSettings); }; - // Add is off when it isn't ready, mid-operation, or the playhead sits on a - // boundary / inside a recorded segment (disableSplit carries both). Kept as - // one value so the button's disabled state and its styling can't drift apart. + // Keep Add disabled state and styling in sync. + // disableSplit covers boundary and recorded-segment cases. const splitDisabled = !ready || busyRef.current || !!disableSplit; const handleSplit = () => { @@ -205,8 +204,7 @@ function WSAudioPlayerSegment(props: IProps) { id="wsSplit" onClick={handleSplit} disabled={splitDisabled} - // Primary (solid) only when it can be used — a disabled Add - // must not draw attention to an action it will refuse (TT-7666). + // Use primary style only when Add is actionable. variant={splitDisabled ? undefined : 'primary'} > diff --git a/src/renderer/src/components/WSAudioPlayerSegmentRecordedLock.test.ts b/src/renderer/src/components/WSAudioPlayerSegmentRecordedLock.test.ts index d23e7df91..90820b517 100644 --- a/src/renderer/src/components/WSAudioPlayerSegmentRecordedLock.test.ts +++ b/src/renderer/src/components/WSAudioPlayerSegmentRecordedLock.test.ts @@ -5,12 +5,10 @@ import { import { IRegion } from '../crud/useWavesurferRegions'; /** - * The +/- segment controls must be disabled — not merely inert — over a - * recorded segment (TT-7666). These are the pure predicates the player uses to - * decide that; the UI feeds them the playhead position and the segment map. + * Tests for +/- blocking rules on recorded segments (TT-7666). */ -// three contiguous 10s segments +// Three contiguous 10-second segments. const regions: IRegion[] = [ { start: 0, end: 10 }, { start: 10, end: 20 }, @@ -22,13 +20,13 @@ const TOL = 0.1; describe('isAddBlockedByRecording — Add (+) over a recorded segment', () => { it('blocks when the playhead is inside a recorded segment', () => { const recorded = (i: number) => i === 1; - // playhead at 15s is inside segment 1 + // 15s is inside segment 1. expect(isAddBlockedByRecording(15, regions, recorded)).toBe(true); }); it('allows when the playhead is inside an unrecorded segment', () => { const recorded = (i: number) => i === 1; - // playhead at 5s is inside segment 0 + // 5s is inside segment 0. expect(isAddBlockedByRecording(5, regions, recorded)).toBe(false); }); @@ -44,25 +42,25 @@ describe('isAddBlockedByRecording — Add (+) over a recorded segment', () => { describe('isRemoveBlockedByRecording — Remove (-) across a recorded boundary', () => { it('blocks at the join when the segment before it is recorded', () => { const recorded = (i: number) => i === 1; - // join between segment 1 and 2 sits at 20s + // Join between segment 1 and 2 is at 20s. expect(isRemoveBlockedByRecording(20, regions, TOL, recorded)).toBe(true); }); it('blocks at the join when the segment after it is recorded', () => { const recorded = (i: number) => i === 2; - // join between segment 1 and 2 at 20s; segment 2 (after) is recorded + // At join 20s, segment 2 (after) is recorded. expect(isRemoveBlockedByRecording(20, regions, TOL, recorded)).toBe(true); }); it('allows at a join between two unrecorded segments', () => { const recorded = (i: number) => i === 2; - // join between segment 0 and 1 at 10s; neither is recorded + // At join 10s, neither side is recorded. expect(isRemoveBlockedByRecording(10, regions, TOL, recorded)).toBe(false); }); it('allows when the playhead is not near any join', () => { const recorded = (i: number) => i === 1; - // 15s is mid-segment, not on a boundary + // 15s is not near a boundary. expect(isRemoveBlockedByRecording(15, regions, TOL, recorded)).toBe(false); }); }); diff --git a/src/renderer/src/components/segmentBoundaryLocks.ts b/src/renderer/src/components/segmentBoundaryLocks.ts index fd7e36554..ff106c791 100644 --- a/src/renderer/src/components/segmentBoundaryLocks.ts +++ b/src/renderer/src/components/segmentBoundaryLocks.ts @@ -1,14 +1,11 @@ import { IRegion } from '../crud/useWavesurferRegions'; /** - * Pure helpers deciding when the player's +/- segment controls must be disabled - * because a recording depends on the boundary (TT-7666). Kept out of - * WSAudioPlayer so they can be unit-tested without the component's app-wide - * import tree. + * Helpers for disabling +/- when a recorded segment would be changed + * (TT-7666). Kept separate for easier unit testing. */ -/** Sorted index of the segment the playhead sits in, or -1. The last segment - * includes its end so the very end of the track still resolves. */ +/** Sorted index of the segment at the playhead, or -1. */ export function segmentIndexAtProgress( progressSec: number, regions: IRegion[] @@ -26,8 +23,7 @@ export function segmentIndexAtProgress( return -1; } -/** Add (+) would split the segment under the playhead; block it when that - * segment is recorded (TT-7666). */ +/** Block Add when it would split a recorded segment (TT-7666). */ export function isAddBlockedByRecording( progressSec: number, regions: IRegion[], @@ -38,8 +34,7 @@ export function isAddBlockedByRecording( return idx >= 0 && isSegmentRecorded(idx); } -/** Remove (−) merges the two segments flanking the internal join near the - * playhead; block it when either is recorded (TT-7666). */ +/** Block Remove when either side of the merged boundary is recorded (TT-7666). */ export function isRemoveBlockedByRecording( progressSec: number, regions: IRegion[], diff --git a/src/renderer/src/crud/useWavesurferRegions.test.tsx b/src/renderer/src/crud/useWavesurferRegions.test.tsx index 157b35b8b..4a7041267 100644 --- a/src/renderer/src/crud/useWavesurferRegions.test.tsx +++ b/src/renderer/src/crud/useWavesurferRegions.test.tsx @@ -1,21 +1,10 @@ import { act, renderHook } from '@testing-library/react'; /** - * Segment-selection lock spec (TT-7437). + * Segment lock spec (TT-7437). * - * While a take is being recorded the selected segment must not move: the take - * belongs to the segment recording started on. Three engine events can move it - * and each one is a separate route the user can take from the waveform: - * - * - `region-clicked` — tapping another segment, - * - `region-in` — the playhead entering another segment after the tap - * seeks it (the click also seeks, so this fires even - * when the click itself was dropped), - * - `region-updated` — dragging a segment boundary. - * - * All three land on `onCurrentRegion`, which is what drives - * PassageDetailContext's currentSegment. So the lock is asserted there rather - * than on any one handler's internals. + * While recording, selection must stay on the start segment. + * We verify all paths that can change selection: click, region-in, and drag. */ // ---- fake wavesurfer regions plugin ----------------------------------------- @@ -32,8 +21,8 @@ interface IFakePlugin { enableDragSelection(): void; } -// The hook picks its plugin out of ws.getActivePlugins() with -// `instanceof RegionsPlugin`, so the fake has to *be* the mocked class. +// The hook finds plugins with `instanceof RegionsPlugin`, so this fake must be +// that mocked class. jest.mock('wavesurfer.js/dist/plugins/regions', () => { class FakeRegionsPlugin { handlers: Record = {}; @@ -52,8 +41,7 @@ jest.mock('wavesurfer.js/dist/plugins/regions', () => { return this.regionList; } addRegion(params: any) { - // wavesurfer hands back a Region, not the params object — the hook - // immediately calls setOptions on it. + // wavesurfer returns a Region object and the hook calls setOptions on it. const r: any = { id: `added-${this.regionList.length}`, attributes: {}, @@ -100,14 +88,11 @@ const DURATION = 30; interface IHarnessOpts { lockSegmentSelection: boolean; - /** Playhead position. A split happens at the playhead, so the double-click - * tests need one that is inside a segment rather than on its edge. */ + /** Playhead position used by split tests. */ progressAt?: number; - /** Sorted indices of segments that already have a recording. A boundary - * between a recorded segment and its neighbor may not be dragged (TT-7666). */ + /** Sorted indices of recorded segments (TT-7666). */ recordedIndices?: number[]; - /** Supply a color function so `applyRegionColors` runs its body (it is a - * no-op without one) — needed to exercise the recorded-resize pass. */ + /** Provide a color function so applyRegionColors runs. */ withColors?: boolean; } @@ -197,14 +182,13 @@ const clickSegment = (plugin: IFakePlugin, r: any) => plugin.emit('region-clicked', r, { stopPropagation: jest.fn() }); }); -// The playhead crosses into a segment (what the tap's seek causes next). +// Playhead enters a segment (for example, after a tap seeks). const playheadEnters = (plugin: IFakePlugin, r: any) => act(() => { plugin.emit('region-in', r); }); -// The user drags a segment boundary and lets go. 'region-update' is what tells -// the hook a resize (rather than a whole-region move) is underway. +// Drag a segment boundary and finish the drag. const dragBoundary = ( plugin: IFakePlugin, r: any, @@ -286,9 +270,7 @@ describe('useWaveSurferRegions — segment selection locked while recording (TT- }); it('does not follow the playhead into another segment', () => { - // The click above is dropped, but a tap on the waveform also seeks. If the - // seek lands in another segment, region-in fires with the lock none the - // wiser — this is the route that kept moving the selection mid-record. + // A tap can still seek and emit region-in. Lock must block that path too. const { plugin, segs, onCurrentRegion } = renderRegions({ lockSegmentSelection: true, }); @@ -309,8 +291,7 @@ describe('useWaveSurferRegions — segment selection locked while recording (TT- }); it('leaves the segment bounds alone when a boundary is dragged', () => { - // Dragging must not silently reshape the segment the take is being - // recorded into either — the take's start/end are what identify it. + // Dragging must not reshape the active take's segment. const { plugin, segs } = renderRegions({ lockSegmentSelection: true }); dragBoundary(plugin, segs[1], 'end', 22); @@ -322,8 +303,7 @@ describe('useWaveSurferRegions — segment selection locked while recording (TT- }); it('does not split a segment on double-click', () => { - // Double-click splits, which reshapes the segment map exactly as a boundary - // drag does — the take's own boundaries would move under it. + // Double-click split also changes boundaries, so it must be blocked. const { plugin, segs } = renderRegions({ lockSegmentSelection: true, progressAt: 15, @@ -348,12 +328,8 @@ describe('useWaveSurferRegions — segment selection locked while recording (TT- }); describe('useWaveSurferRegions — boundary drag on a recorded segment (TT-7666)', () => { - // A recording is tied to a segment's exact time range, so once a segment has - // a Phrase BT (or Careful Speech) recording its boundaries are frozen. A - // boundary is shared by two segments, so a drag is refused when *either* - // side of it is recorded — this is separate from the recording-in-progress - // lock: it holds whenever the neighbouring recording exists, recording or - // not. + // Recorded boundaries are frozen (TT-7666). Since a boundary is shared, + // drag is blocked when either neighboring segment is recorded. it('refuses to move a recorded segment via its own boundary', () => { // Segment 1 is recorded. Dragging its end would resize it. @@ -364,15 +340,13 @@ describe('useWaveSurferRegions — boundary drag on a recorded segment (TT-7666) dragBoundary(plugin, segs[1], 'end', 22); - // The shared neighbour was not pulled along, and nothing downstream heard a - // boundary change. + // Neighbor stays unchanged and no update is emitted. expect(segs[2].start).toBe(20); expect(onCurrentRegion).not.toHaveBeenCalled(); }); it('refuses to move a recorded neighbour via the shared boundary', () => { - // Segment 1 is unrecorded but segment 2 is recorded; dragging segment 1's - // end drags segment 2's start with it, which would reshape the recording. + // Dragging this edge would also move recorded segment 2, so block it. const { plugin, segs, onCurrentRegion } = renderRegions({ lockSegmentSelection: false, recordedIndices: [2], @@ -398,9 +372,7 @@ describe('useWaveSurferRegions — boundary drag on a recorded segment (TT-7666) }); it('still allows dragging a boundary between two unrecorded segments', () => { - // Segment 2 is recorded, but segment 1's *start* boundary is shared with - // segment 0 — both unrecorded — so it must stay draggable. The lock is - // per-boundary, not "any recording nearby freezes everything". + // This boundary is between unrecorded segments, so it stays draggable. const { plugin, segs, onCurrentRegion } = renderRegions({ lockSegmentSelection: false, recordedIndices: [2], @@ -415,9 +387,7 @@ describe('useWaveSurferRegions — boundary drag on a recorded segment (TT-7666) }); it('freezes a recorded segment but keeps its boundary handles visible', () => { - // The handle is the only clear marker of where a segment ends, so it stays - // rendered (resize: true) — but both edges are turned off so it cannot be - // dragged (TT-7666). + // Keep the handle visible, but disable dragging on both sides (TT-7666). const { segs } = renderRegions({ lockSegmentSelection: false, recordedIndices: [1], @@ -451,10 +421,7 @@ describe('useWaveSurferRegions — boundary drag on a recorded segment (TT-7666) }); it('does not re-enable handles on a color pass while recording is locked', () => { - // The recording-in-progress lock (TT-7437) takes every region's handles - // away. The recorded-resize pass runs on every color update and must not - // hand a handle back to an unrecorded segment while that lock holds, or the - // user could resize mid-record. + // While lock is active, color updates must not re-enable drag handles. const { result, segs } = renderRegions({ lockSegmentSelection: true, recordedIndices: [1], @@ -466,7 +433,7 @@ describe('useWaveSurferRegions — boundary drag on a recorded segment (TT-7666) result.current.applyRegionColors(); }); - // segment 0 is unrecorded, but the lock is up: it must not be re-enabled. + // Segment 0 is unrecorded, but lock still forbids re-enable. expect(segs[0].setOptions).not.toHaveBeenCalledWith( expect.objectContaining({ resize: true }) ); @@ -474,10 +441,8 @@ describe('useWaveSurferRegions — boundary drag on a recorded segment (TT-7666) }); describe('useWaveSurferRegions — the +/- controls on a recorded segment (TT-7666)', () => { - // The Add (+) button splits the segment under the playhead; Remove (-) merges - // the current segment with a neighbour. Neither may touch a recorded segment, - // and a refused click must be inert — no divider added or removed, and the - // playhead left where it was. + // Add splits and Remove merges. Neither may modify recorded segments. + // Blocked actions should do nothing. it('adds a divider inside an unrecorded segment (control)', () => { const { result, plugin, goto } = renderRegions({ diff --git a/src/renderer/src/crud/useWavesurferRegions.tsx b/src/renderer/src/crud/useWavesurferRegions.tsx index 7697f81bd..90d7f2636 100644 --- a/src/renderer/src/crud/useWavesurferRegions.tsx +++ b/src/renderer/src/crud/useWavesurferRegions.tsx @@ -139,11 +139,8 @@ export function useWaveSurferRegions( */ onRegionClicked?: (region: IRegion) => void, /** - * Whether the segment at the given sorted index already has a recording. A - * recording is tied to a segment's exact time range, so a boundary shared - * with a recorded segment may not be dragged — regardless of the - * recording-in-progress lock (TT-7666). Keyed on sorted index, like - * applyRegionColor, because that is how consumers track completion. + * Whether a sorted segment index already has a recording (TT-7666). + * Boundaries shared with recorded segments are not draggable. */ isSegmentRecorded?: (sortedIndex: number) => boolean ) { @@ -169,8 +166,7 @@ export function useWaveSurferRegions( applyRegionColor ); const lockSegmentSelectionRef = useRef(lockSegmentSelection ?? false); - /** Regions whose drag/resize were taken away by the lock, with the flags to - * give back when it lifts. */ + /** Regions frozen by the lock, plus the drag/resize flags to restore later. */ const dragLockedRegionsRef = useRef< { region: Region; resize: boolean; drag: boolean }[] >([]); @@ -208,8 +204,7 @@ export function useWaveSurferRegions( applyRegionColorRef.current = applyRegionColor; }, [applyRegionColor]); - /** Take a region's drag/resize away for the duration of the lock, remembering - * the flags it had so unlock gives back exactly those. */ + /** Disable drag/resize for this region and remember prior flags for unlock. */ const freezeRegionDrag = (r: Region) => { if (dragLockedRegionsRef.current.some((e) => e.region === r)) return; dragLockedRegionsRef.current.push({ @@ -223,14 +218,9 @@ export function useWaveSurferRegions( useEffect(() => { 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. + // While recording lock is on, freeze drag/resize for every region. + // This keeps both selection and boundaries stable for the active take + // (TT-7437). On unlock, restore each region's original flags. if (locked) { regions().forEach(freezeRegionDrag); } else { @@ -238,9 +228,8 @@ export function useWaveSurferRegions( r.setOptions({ resize, drag }) ); dragLockedRegionsRef.current = []; - // The snapshot restores the flags as they were at lock time; now re-derive - // the recorded/unrecorded resting state, since a take may have been made - // during the lock (TT-7666). No-op while locked, so it is safe here only. + // Reapply recorded boundary locks because recording state may have + // changed while the lock was active (TT-7666). applyRecordedResizeLocks(); } // regions() reads a ref, so it needs no dep of its own. @@ -291,10 +280,7 @@ export function useWaveSurferRegions( applyRecordedResizeLocks(); }; - /** - * The ew-resize cursor wavesurfer puts on a resize handle. Overridden to the - * default cursor on a frozen boundary so it doesn't invite a drag it refuses. - */ + /** Show ew-resize only on draggable handles; otherwise show default cursor. */ const setHandleCursor = ( r: Region, side: 'left' | 'right', @@ -307,33 +293,24 @@ export function useWaveSurferRegions( }; /** - * Freeze any boundary a recording depends on (TT-7666). A recording is tied to - * a segment's exact time range, so once a segment is recorded its edges can't - * move — and a boundary is shared by two segments, so both sides of it are - * frozen. The handle stays *visible* (it is the only clear marker of where a - * segment ends), but its resize is turned off and its cursor reverts to the - * default so it does not look draggable. Recomputed on every color pass (the - * same trigger as green/pending), so deleting a take unfreezes it. Idempotent: - * each region's full desired state is set every time. + * Freeze boundaries that touch recorded segments (TT-7666). + * Handles stay visible, but non-draggable sides are disabled. */ const applyRecordedResizeLocks = () => { const recorded = isSegmentRecordedRef.current; if (!recorded) return; - // While the recording-in-progress lock holds it owns every region's resize - // flag; re-enabling any here would fight it. The lock's release path re-runs - // this to restore the resting recorded/unrecorded state (TT-7437 / TT-7666). + // Do not re-enable handles while recording lock owns region flags. + // The unlock path reruns this and restores final state. if (lockSegmentSelectionRef.current) return; const sorted = sortedRegions(); const last = sorted.length - 1; sorted.forEach((r, i) => { - // A side is draggable only when neither the segment nor the neighbour - // across that side is recorded. The outer edges (first start, last end) - // have no neighbour, so the predicate is never asked an out-of-range index. + // A side is draggable only when neither this segment nor the neighbor on + // that side is recorded. const self = recorded(i); const startActive = !self && (i === 0 || !recorded(i - 1)); const endActive = !self && (i === last || !recorded(i + 1)); - // resize stays true so the handles (and thus the boundary lines) render; - // resizeStart/resizeEnd gate the actual drag per side. + // Keep handles visible; gate drag by side with resizeStart/resizeEnd. r.setOptions({ resize: true, resizeStart: startActive, @@ -344,20 +321,14 @@ export function useWaveSurferRegions( }); }; - /** - * Whether the boundary the user is dragging on region `r` (its `side` edge, - * or either edge when the side is unknown) is shared with a recorded segment - * — in which case the drag must be refused (TT-7666). - */ + /** True when the dragged boundary touches a recorded segment (TT-7666). */ const isRecordedBoundary = (r: Region, side?: UpdateSide) => { const recorded = isSegmentRecordedRef.current; if (!recorded) return false; const idx = regionIndexInSorted(r); if (idx < 0) return false; if (recorded(idx)) return true; - // 'start' shares with the previous segment, 'end' with the next; an - // unknown side means check both. Guard the ends so the predicate is only - // ever asked about a real sorted index. + // 'start' shares previous, 'end' shares next; unknown side checks both. if (side !== 'end' && idx > 0 && recorded(idx - 1)) return true; if (side !== 'start' && idx < numRegions() - 1 && recorded(idx + 1)) { return true; @@ -447,9 +418,7 @@ export function useWaveSurferRegions( // handle region double-clicks with deduplication // This is an event handler, not a render function, so Date.now() is safe here const handleRegionDoubleClick = (r: Region) => { - // Double-click splits the segment. That reshapes the segment map under a - // take in progress just as a boundary drag would, so it is locked with the - // rest of them (TT-7437). + // Double-click split is blocked during recording lock (TT-7437). if (lockSegmentSelectionRef.current) return; const currentTime = getCurrentTime(); const timeSinceLastDoubleClick = @@ -625,10 +594,7 @@ export function useWaveSurferRegions( regionsPlugin.on('region-created', function (r: Region) { if (isMarker(r)) return; r.drag = singleRegionRef.current; - // A region born while the lock is up — the waveform loading with the - // lock already on, or a reload during it — never went through the - // freeze in the lock effect, so it would arrive draggable. Freeze it - // here instead, where every region passes exactly once (TT-7437). + // If region is created while lock is active, freeze it immediately. if (lockSegmentSelectionRef.current) freezeRegionDrag(r); // Round region start and end to 5 decimal places because the seek uses 5 decimal places @@ -697,8 +663,8 @@ export function useWaveSurferRegions( 'region-update', function (r: Region, side?: UpdateSide) { if (lockSegmentSelectionRef.current) return; - // A recorded segment's boundary is frozen; the handle is gone, so - // this is the backstop for a gesture already in flight (TT-7666). + // Backstop: if a drag was already in flight, still block recorded + // boundaries here (TT-7666). if (isRecordedBoundary(r, side)) return; resizingRef.current = r.resize; // Live-clamp the boundary as the user drags so regions never visually @@ -712,17 +678,10 @@ export function useWaveSurferRegions( regionsPlugin.on( 'region-updated', function (r: Region, side?: UpdateSide) { - // When the user finishes dragging a segment edge, this selects the - // segment they dragged and moves the neighbor's edge to match. Both - // are things a recording in progress must not have happen to it: the - // take belongs to one segment, at the size it was when recording - // started (TT-7437). - // - // While locked, the segments can't be dragged at all, so we only get - // here if the user was already dragging when recording began. + // region-updated can change selection and boundaries, so block it + // while recording lock is active (TT-7437). if (lockSegmentSelectionRef.current) return; - // Same for a boundary a recording depends on (TT-7666): its handle is - // gone, so reaching here means a drag was already under way. + // Backstop for in-flight drags on recorded boundaries (TT-7666). if (isRecordedBoundary(r, side)) return; if (singleRegionRef.current) { if (!loadingRef.current) { @@ -764,13 +723,8 @@ export function useWaveSurferRegions( // Ignore region-in for any region other than the one we're targeting so // the adjacent segment isn't spuriously selected. if (playRegionRef.current && r.id !== playRegionRef.current.id) return; - // The lock applies here too. A click on the waveform is both a click - // and a seek: region-clicked is dropped below, but the seek still walks - // the playhead into the clicked segment and region-in fires behind the - // lock's back. That is how the selection kept moving mid-record even - // with the click blocked (TT-7437). Nothing legitimate is lost — - // recording forces playback off, so there is no playback or overshoot - // to track while the lock is up. + // A click also seeks, which can trigger region-in; block this too so + // selection cannot move during recording lock (TT-7437). if (lockSegmentSelectionRef.current) return; if (!loopingRef.current) setCurrentRegion(r); }); @@ -841,7 +795,7 @@ export function useWaveSurferRegions( color: 'rgba(255, 0, 0, 0.1)', }); } - // Freeze the handles of any already-recorded segment (TT-7666). + // Apply recorded-boundary locks for existing segments (TT-7666). applyRecordedResizeLocks(); } }; @@ -1132,7 +1086,7 @@ export function useWaveSurferRegions( region.id = r?.id; }); setPrevNext(regarray.map((r: any) => r.id)); - // Freeze the handles of recorded segments in the freshly loaded map (TT-7666). + // Re-apply recorded-boundary locks after loading regions (TT-7666). applyRecordedResizeLocks(); onRegion(regarray.length, newRegions); onRegionGoTo(regarray[defaultRegionIndex]?.start ?? 0); @@ -1235,9 +1189,8 @@ export function useWaveSurferRegions( clearRegions(); return; } - // Removing a boundary merges two segments; refuse if either is recorded — - // it would reshape the recording's segment (TT-7666). Same neighbour choice - // as the merge below (playhead near the start merges with prev, else next). + // Removing a boundary merges two segments. Block it if either side is + // recorded (TT-7666). const recorded = isSegmentRecordedRef.current; if (recorded) { const mergePrev = findPrevRegion(r); @@ -1299,8 +1252,7 @@ export function useWaveSurferRegions( const wsAddRegion = () => { const target = findRegion(progress(), true); - // No new boundary inside a recorded segment — that would split its audio in - // two (TT-7666). Return without splitting or moving the playhead. + // Do not split inside a recorded segment (TT-7666). if (target && isSegmentRecordedRef.current?.(regionIndexInSorted(target))) { return undefined; } From 21df1401ab3f1c76e53790a75202b1a337f57f53 Mon Sep 17 00:00:00 2001 From: Noel Chou Date: Fri, 4 Sep 2026 08:35:57 -0400 Subject: [PATCH 12/27] TT-7437 apply the recording lock to the +/- segment tools MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit 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) --- .../src/crud/useWavesurferRegions.test.tsx | 19 +++++++++++++++++++ .../src/crud/useWavesurferRegions.tsx | 7 +++++++ 2 files changed, 26 insertions(+) diff --git a/src/renderer/src/crud/useWavesurferRegions.test.tsx b/src/renderer/src/crud/useWavesurferRegions.test.tsx index 4a7041267..be3ffad30 100644 --- a/src/renderer/src/crud/useWavesurferRegions.test.tsx +++ b/src/renderer/src/crud/useWavesurferRegions.test.tsx @@ -325,6 +325,25 @@ describe('useWaveSurferRegions — segment selection locked while recording (TT- expect(result.current.wsPrevRegion()).toBe(false); expect(result.current.wsNextRegion()).toBe(false); }); + + it('refuses Add while recording, before the take is saved', () => { + // The in-progress segment is not yet in the recorded set, so only the lock + // stops +/- from splitting the take mid-record (TT-7437). + const { result, plugin, goto } = renderRegions({ + lockSegmentSelection: true, + progressAt: 15, // inside segment 1 + }); + const before = plugin.regionList.length; + + let ret: unknown; + act(() => { + ret = result.current.wsAddRegion(); + }); + + expect(ret).toBeUndefined(); + expect(plugin.regionList.length).toBe(before); // no divider added + expect(goto).not.toHaveBeenCalled(); // playhead not moved + }); }); describe('useWaveSurferRegions — boundary drag on a recorded segment (TT-7666)', () => { diff --git a/src/renderer/src/crud/useWavesurferRegions.tsx b/src/renderer/src/crud/useWavesurferRegions.tsx index 90d7f2636..7e9e829e2 100644 --- a/src/renderer/src/crud/useWavesurferRegions.tsx +++ b/src/renderer/src/crud/useWavesurferRegions.tsx @@ -1183,6 +1183,10 @@ export function useWaveSurferRegions( }; const wsRemoveSplitRegion = () => { + // No boundary edits while a take is being recorded or saved (TT-7437). The + // in-progress segment is not yet in the recorded set, so the isSegmentRecorded + // check below would not catch it — the lock is what covers the active take. + if (lockSegmentSelectionRef.current) return undefined; const r = currentRegion(); if (!r) return undefined; if (numRegions() === 1) { @@ -1251,6 +1255,9 @@ export function useWaveSurferRegions( }; const wsAddRegion = () => { + // No boundary edits while a take is being recorded or saved (TT-7437) — the + // in-progress segment is not yet in the recorded set the check below reads. + if (lockSegmentSelectionRef.current) return undefined; const target = findRegion(progress(), true); // Do not split inside a recorded segment (TT-7666). if (target && isSegmentRecordedRef.current?.(regionIndexInSorted(target))) { From 6e475944815b29cf8cf47a60b3ec5137013502e1 Mon Sep 17 00:00:00 2001 From: Noel Chou Date: Fri, 4 Sep 2026 09:58:24 -0400 Subject: [PATCH 13/27] TT-7666 one reactive recorded-state for every boundary guard; handles always visible MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit 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) --- .../PassageDetailCarefulSpeech.test.tsx | 47 ++++++ .../PassageDetailGuidedPhraseRecord.tsx | 81 ++++++++--- .../src/crud/useWavesurferRegions.test.tsx | 136 +++++++++++++----- .../src/crud/useWavesurferRegions.tsx | 95 ++++++------ 4 files changed, 257 insertions(+), 102 deletions(-) diff --git a/src/renderer/src/components/PassageDetail/PassageDetailCarefulSpeech.test.tsx b/src/renderer/src/components/PassageDetail/PassageDetailCarefulSpeech.test.tsx index 82640d3a8..9f75b98d1 100644 --- a/src/renderer/src/components/PassageDetail/PassageDetailCarefulSpeech.test.tsx +++ b/src/renderer/src/components/PassageDetail/PassageDetailCarefulSpeech.test.tsx @@ -751,3 +751,50 @@ describe('PassageDetailCarefulSpeech — rejected save (TT-7583)', () => { expect(screen.queryByRole('button', { name: 'Retry' })).toBeNull(); }); }); + +describe('PassageDetailCarefulSpeech — recorded-state consistency across guards (TT-7666)', () => { + // Every boundary-editing guard must read the same recorded view: the drag / + // +- guards (via isSegmentRecorded on the player) and Split/Combine (via their + // can* props) agree, including the optimistic window right after a save, and + // all re-enable together when the take is removed. + const recordClause2 = async () => { + mockCompleted = new Set([0, 1]); // auto-play clause 2 + await mountAndSettle(); + await firePlaybackEnd(2); + await act(async () => { + (controlsProps?.onRecording as (active: boolean) => void)(true); + (controlsProps?.onRecording as (active: boolean) => void)(false); + }); + await act(async () => { + await ( + controlsProps?.afterUploadCb as ( + mediaId: string | undefined + ) => Promise + )('media-new'); + }); + }; + + it('treats a just-saved clause as recorded for drag and for Combine alike', async () => { + await recordClause2(); + + const isRecorded = playerProps?.isSegmentRecorded as (i: number) => boolean; + // drag / +- guard sees clause 2 recorded (optimistic, rowData still lagging) + expect(isRecorded(2)).toBe(true); + // ...and Combine agrees — it is disabled on the same clause. + expect(controlsProps?.canCombineWithNext).toBe(false); + }); + + it('re-enables drag and Combine together when the take is cleared', async () => { + await recordClause2(); + mockRecordingRow = undefined; // nothing persisted; clearing drops the take + + await act(async () => { + (controlsProps?.onClearRecording as () => void)(); + }); + + const isRecorded = playerProps?.isSegmentRecorded as (i: number) => boolean; + // Both guards let go of clause 2 at once — no asymmetry (the reported bug). + expect(isRecorded(2)).toBe(false); + expect(controlsProps?.canCombineWithNext).toBe(true); + }); +}); diff --git a/src/renderer/src/components/PassageDetail/PassageDetailGuidedPhraseRecord.tsx b/src/renderer/src/components/PassageDetail/PassageDetailGuidedPhraseRecord.tsx index 0b010ef03..bea3eb57d 100644 --- a/src/renderer/src/components/PassageDetail/PassageDetailGuidedPhraseRecord.tsx +++ b/src/renderer/src/components/PassageDetail/PassageDetailGuidedPhraseRecord.tsx @@ -258,8 +258,33 @@ export function PassageDetailGuidedPhraseRecord({ // clause; this lets the recording effect swallow that single +1 advance while // still treating any non-adjacent jump as a genuine user tap (TT-7360). const pendingOvershootSwallowRef = useRef(false); - /** Indices saved this session whose rowData may not have caught up yet (TT-7552). */ + /** Indices saved this session whose rowData may not have caught up yet (TT-7552). + * Read synchronously (coloring) via the ref; the version below makes changes + * to it reactive so the recorded-aware guards recompute (TT-7666). */ const optimisticCompletedRef = useRef>(new Set()); + const [optimisticVersion, setOptimisticVersion] = useState(0); + const bumpOptimistic = useCallback( + () => setOptimisticVersion((v) => v + 1), + [] + ); + const addOptimistic = useCallback( + (index: number) => { + optimisticCompletedRef.current.add(index); + bumpOptimistic(); + }, + [bumpOptimistic] + ); + const removeOptimistic = useCallback( + (index: number) => { + if (optimisticCompletedRef.current.delete(index)) bumpOptimistic(); + }, + [bumpOptimistic] + ); + const clearOptimistic = useCallback(() => { + if (optimisticCompletedRef.current.size === 0) return; + optimisticCompletedRef.current.clear(); + bumpOptimistic(); + }, [bumpOptimistic]); const currentIndexRef = useRef(0); /** * Clause and region for the active take (TT-7437). @@ -487,7 +512,19 @@ export function PassageDetailGuidedPhraseRecord({ recordingPassStarted && (completedIndices.has(index) || optimisticCompletedRef.current.has(index)), - [recordingPassStarted, completedIndices] + // optimisticVersion makes optimistic-set changes reactive so every guard + // that reads this predicate (drag, +/-, Split/Combine) recomputes together. + // eslint-disable-next-line react-hooks/exhaustive-deps + [recordingPassStarted, completedIndices, optimisticVersion] + ); + + /** completedIndices plus the optimistic just-saved set — the single recorded + * view every boundary-editing guard uses, so they agree in the window before + * rowData catches up (TT-7666). */ + const recordedForTools = useMemo( + () => new Set([...completedIndices, ...optimisticCompletedRef.current]), + // eslint-disable-next-line react-hooks/exhaustive-deps + [completedIndices, optimisticVersion] ); const allClausesComplete = useMemo( @@ -611,8 +648,11 @@ export function PassageDetailGuidedPhraseRecord({ changed = true; } } - if (changed) applyColors(); - }, [completedIndices, applyColors]); + if (changed) { + bumpOptimistic(); + applyColors(); + } + }, [completedIndices, applyColors, bumpOptimistic]); const bumpSuppressClauseAutoPlay = useCallback((count = 1) => { suppressClauseAutoPlayRef.current += count; @@ -967,7 +1007,7 @@ export function PassageDetailGuidedPhraseRecord({ // The latch is mediafile-specific. Clear it when source media changes, // so the next take cannot reuse a clause from the old waveform (TT-7437). latchRecordingTarget(undefined); - optimisticCompletedRef.current.clear(); + clearOptimistic(); setHeardIndices([]); setCurrentClausePlayed(false); setCombineUndo(null); @@ -1432,7 +1472,7 @@ export function PassageDetailGuidedPhraseRecord({ playerControlsRef.current?.loadRegionsJson?.(baseline); setRecordingPassStarted(false); recordingPassStartedRef.current = false; - optimisticCompletedRef.current.clear(); + clearOptimistic(); setShowRecorder(false); setHeardIndices([]); setCurrentClausePlayed(false); @@ -1467,6 +1507,7 @@ export function PassageDetailGuidedPhraseRecord({ setStepComplete, forceRefresh, applyColors, + clearOptimistic, ]); const handleClearSegments = useCallback(async () => { @@ -1513,7 +1554,7 @@ export function PassageDetailGuidedPhraseRecord({ phraseSegParams ); if ( - !canSplitClause(currentIndex, clauseRegions, completedIndices, splitPoint) + !canSplitClause(currentIndex, clauseRegions, recordedForTools, splitPoint) ) { return; } @@ -1537,7 +1578,7 @@ export function PassageDetailGuidedPhraseRecord({ }, [ currentIndex, clauseRegions, - completedIndices, + recordedForTools, clauseSegString, phraseSegParams, setClauseSegString, @@ -1551,7 +1592,7 @@ export function PassageDetailGuidedPhraseRecord({ const handleCombineWithNext = useCallback(async () => { if (savingRecordingRef.current) return; - if (!canCombineWithNext(currentIndex, clauseRegions, completedIndices)) { + if (!canCombineWithNext(currentIndex, clauseRegions, recordedForTools)) { return; } const updated = mergeClauseWithNext(clauseRegions, currentIndex); @@ -1570,7 +1611,7 @@ export function PassageDetailGuidedPhraseRecord({ }, [ currentIndex, clauseRegions, - completedIndices, + recordedForTools, clauseSegString, phraseSegParams, setClauseSegString, @@ -1766,11 +1807,11 @@ export function PassageDetailGuidedPhraseRecord({ const takenIndex = recordingTargetRef.current?.index ?? currentIndexRef.current; if (mediaId) { - optimisticCompletedRef.current.add(takenIndex); + addOptimistic(takenIndex); // Stored: the take is no longer pending, so release the clause. latchRecordingTarget(undefined); } else { - optimisticCompletedRef.current.delete(takenIndex); + removeOptimistic(takenIndex); // Keep the latch on failed upload so Retry files to the same clause // even if selection moved (TT-7583). } @@ -1784,7 +1825,13 @@ export function PassageDetailGuidedPhraseRecord({ setResetMedia(false); applyColors(); }, - [forceRefresh, applyColors, latchRecordingTarget] + [ + forceRefresh, + applyColors, + latchRecordingTarget, + addOptimistic, + removeOptimistic, + ] ); const handleClearRecording = useCallback(async () => { @@ -1805,7 +1852,7 @@ export function PassageDetailGuidedPhraseRecord({ await setStepComplete(currentstep, false); } } - optimisticCompletedRef.current.delete( + removeOptimistic( recordingTargetRef.current?.index ?? currentIndexRef.current ); // The take is gone, so the clause it was held against is released too. @@ -1973,13 +2020,13 @@ export function PassageDetailGuidedPhraseRecord({ canSplitClause={canSplitClause( currentIndex, clauseRegions, - completedIndices, + recordedForTools, currentClauseSplitPoint )} canCombineWithNext={canCombineWithNext( currentIndex, clauseRegions, - completedIndices + recordedForTools )} showUndoCombine={ combineUndo !== null && !config.multiLevelSegmentUndo @@ -2054,7 +2101,7 @@ export function PassageDetailGuidedPhraseRecord({ // MediaRecord's save-requested-with-no-audio branch only lands here, // Also clear optimistic green on this failure path (TT-7583). // Keep the latch so Retry still files to the same clause. - optimisticCompletedRef.current.delete( + removeOptimistic( recordingTargetRef.current?.index ?? currentIndexRef.current ); applyColors(); diff --git a/src/renderer/src/crud/useWavesurferRegions.test.tsx b/src/renderer/src/crud/useWavesurferRegions.test.tsx index be3ffad30..e7db0e7f8 100644 --- a/src/renderer/src/crud/useWavesurferRegions.test.tsx +++ b/src/renderer/src/crud/useWavesurferRegions.test.tsx @@ -128,42 +128,58 @@ const renderRegions = ({ const goto = jest.fn(); const onRegion = jest.fn(); const setPlaying = jest.fn(); - const isSegmentRecorded = jest.fn((index: number) => - recordedIndices.includes(index) - ); - const { result } = renderHook(() => - useWaveSurferRegions( - false, // singleRegionOnly — Careful Speech is multi-region - 0, - ws, - { current: undefined }, - onRegion, - () => DURATION, - () => false, - goto, - () => progressAt, - () => false, - setPlaying, - onCurrentRegion, - undefined, // onStartRegion - undefined, // onRegionPlayEnd - undefined, // onMarkerClick - undefined, // verses - undefined, // hasSegmentUndo - withColors ? () => 'rgba(1, 2, 3, 0.5)' : undefined, // applyRegionColor - lockSegmentSelection, - undefined, // getDecodedBuffer - true, // disableDragSelection - onRegionClicked, - isSegmentRecorded - ) + // Props threaded through renderHook so a rerender models the real reactivity: + // a fresh isSegmentRecorded identity (when recordings change) and a new lock + // value both re-run the hook's effects. + const { result, rerender } = renderHook( + ({ lock, recorded }: { lock: boolean; recorded: number[] }) => + useWaveSurferRegions( + false, // singleRegionOnly — Careful Speech is multi-region + 0, + ws, + { current: undefined }, + onRegion, + () => DURATION, + () => false, + goto, + () => progressAt, + () => false, + setPlaying, + onCurrentRegion, + undefined, // onStartRegion + undefined, // onRegionPlayEnd + undefined, // onMarkerClick + undefined, // verses + undefined, // hasSegmentUndo + withColors ? () => 'rgba(1, 2, 3, 0.5)' : undefined, // applyRegionColor + lock, + undefined, // getDecodedBuffer + true, // disableDragSelection + onRegionClicked, + (index: number) => recorded.includes(index) + ), + { + initialProps: { + lock: lockSegmentSelection, + recorded: recordedIndices, + }, + } ); act(() => { result.current.setupRegions(ws); }); + /** Re-run the hook with a changed lock and/or recorded set. */ + const update = (next: { lock?: boolean; recorded?: number[] }) => + act(() => { + rerender({ + lock: next.lock ?? lockSegmentSelection, + recorded: next.recorded ?? recordedIndices, + }); + }); + return { result, plugin, @@ -172,7 +188,7 @@ const renderRegions = ({ onRegionClicked, goto, setPlaying, - isSegmentRecorded, + update, }; }; @@ -344,6 +360,33 @@ describe('useWaveSurferRegions — segment selection locked while recording (TT- expect(plugin.regionList.length).toBe(before); // no divider added expect(goto).not.toHaveBeenCalled(); // playhead not moved }); + + it('keeps the boundary handles visible while locked', () => { + // Handles are the only marker of where a segment ends; the lock must never + // remove them (it disables the drag per side instead). + const { segs } = renderRegions({ + lockSegmentSelection: true, + withColors: true, + }); + + segs.forEach((s) => { + expect(s.setOptions).not.toHaveBeenCalledWith( + expect.objectContaining({ resize: false }) + ); + }); + }); + + it('freezes both sides of every segment while locked', () => { + const { segs } = renderRegions({ + lockSegmentSelection: true, + withColors: true, + }); + + // A middle segment: both edges inert during the lock. + expect(segs[1].setOptions).toHaveBeenCalledWith( + expect.objectContaining({ resizeStart: false, resizeEnd: false }) + ); + }); }); describe('useWaveSurferRegions — boundary drag on a recorded segment (TT-7666)', () => { @@ -439,8 +482,9 @@ describe('useWaveSurferRegions — boundary drag on a recorded segment (TT-7666) ); }); - it('does not re-enable handles on a color pass while recording is locked', () => { - // While lock is active, color updates must not re-enable drag handles. + it('does not make an unrecorded segment draggable on a color pass while locked', () => { + // The lock forbids dragging every segment. A color update must not re-arm a + // side — but it must still leave the handles visible (resize stays true). const { result, segs } = renderRegions({ lockSegmentSelection: true, recordedIndices: [1], @@ -452,9 +496,33 @@ describe('useWaveSurferRegions — boundary drag on a recorded segment (TT-7666) result.current.applyRegionColors(); }); - // Segment 0 is unrecorded, but lock still forbids re-enable. + // Segment 0 is unrecorded, but the lock keeps both its edges inert. + expect(segs[0].setOptions).not.toHaveBeenCalledWith( + expect.objectContaining({ resizeStart: true }) + ); + expect(segs[0].setOptions).not.toHaveBeenCalledWith( + expect.objectContaining({ resizeEnd: true }) + ); + // ...and never removes the handle. expect(segs[0].setOptions).not.toHaveBeenCalledWith( - expect.objectContaining({ resize: true }) + expect.objectContaining({ resize: false }) + ); + }); + + it('re-enables a boundary when its recording is removed', () => { + // Consistency + the delete case: once the take is gone, the segment's edges + // come back — the same way Combine/Split re-enable. + const { segs, update } = renderRegions({ + lockSegmentSelection: false, + recordedIndices: [1], + withColors: true, + }); + segs.forEach((s) => s.setOptions.mockClear()); + + update({ recorded: [] }); // recording deleted + + expect(segs[1].setOptions).toHaveBeenCalledWith( + expect.objectContaining({ resizeStart: true, resizeEnd: true }) ); }); }); diff --git a/src/renderer/src/crud/useWavesurferRegions.tsx b/src/renderer/src/crud/useWavesurferRegions.tsx index 7e9e829e2..52d9c3d5b 100644 --- a/src/renderer/src/crud/useWavesurferRegions.tsx +++ b/src/renderer/src/crud/useWavesurferRegions.tsx @@ -166,10 +166,6 @@ export function useWaveSurferRegions( applyRegionColor ); const lockSegmentSelectionRef = useRef(lockSegmentSelection ?? false); - /** Regions frozen by the lock, plus the drag/resize flags to restore later. */ - const dragLockedRegionsRef = useRef< - { region: Region; resize: boolean; drag: boolean }[] - >([]); // setupRegions runs from a once-registered 'ready' handler, so it can only // reach this prop through a ref (like singleRegionRef). const disableDragSelectionRef = useRef(disableDragSelection ?? false); @@ -204,37 +200,18 @@ export function useWaveSurferRegions( applyRegionColorRef.current = applyRegionColor; }, [applyRegionColor]); - /** Disable drag/resize for this region and remember prior flags for unlock. */ - const freezeRegionDrag = (r: Region) => { - if (dragLockedRegionsRef.current.some((e) => e.region === r)) return; - dragLockedRegionsRef.current.push({ - region: r, - resize: r.resize, - drag: r.drag, - }); - r.setOptions({ resize: false, drag: false }); - }; - + // One reactive pass owns which boundaries can be dragged. It re-runs whenever + // the recording lock (TT-7437) or the recorded-segment set (TT-7666) changes, + // so freezing and un-freezing — e.g. a boundary coming back after its + // recording is deleted — happen the same way, on the same signal, as the + // Split/Combine and +/- guards that read the same state. Handles are never + // removed; only their per-side drag is toggled (see applyBoundaryEditability). useEffect(() => { - const locked = lockSegmentSelection ?? false; - lockSegmentSelectionRef.current = locked; - // While recording lock is on, freeze drag/resize for every region. - // This keeps both selection and boundaries stable for the active take - // (TT-7437). On unlock, restore each region's original flags. - if (locked) { - regions().forEach(freezeRegionDrag); - } else { - dragLockedRegionsRef.current.forEach(({ region: r, resize, drag }) => - r.setOptions({ resize, drag }) - ); - dragLockedRegionsRef.current = []; - // Reapply recorded boundary locks because recording state may have - // changed while the lock was active (TT-7666). - applyRecordedResizeLocks(); - } - // regions() reads a ref, so it needs no dep of its own. + lockSegmentSelectionRef.current = lockSegmentSelection ?? false; + applyBoundaryEditability(); + // applyBoundaryEditability reads refs; deps are the two reactive inputs. // eslint-disable-next-line react-hooks/exhaustive-deps - }, [lockSegmentSelection]); + }, [lockSegmentSelection, isSegmentRecorded]); useEffect(() => { disableDragSelectionRef.current = disableDragSelection ?? false; @@ -277,7 +254,7 @@ export function useWaveSurferRegions( r.setOptions({ color: base }); } }); - applyRecordedResizeLocks(); + applyBoundaryEditability(); }; /** Show ew-resize only on draggable handles; otherwise show default cursor. */ @@ -293,24 +270,36 @@ export function useWaveSurferRegions( }; /** - * Freeze boundaries that touch recorded segments (TT-7666). - * Handles stay visible, but non-draggable sides are disabled. + * The single source of truth for which boundaries can be dragged. A side is + * draggable unless the recording lock is up (TT-7437) or the segment — or its + * neighbour across that side — is recorded (TT-7666). Handles are always left + * visible (resize: true); only the per-side drag (resizeStart/resizeEnd) and + * the cursor change, so a boundary marker never disappears. Idempotent, so it + * is safe to run on every color pass, region load, and lock/recorded change. */ - const applyRecordedResizeLocks = () => { + const applyBoundaryEditability = () => { + const locked = lockSegmentSelectionRef.current; const recorded = isSegmentRecordedRef.current; - if (!recorded) return; - // Do not re-enable handles while recording lock owns region flags. - // The unlock path reruns this and restores final state. - if (lockSegmentSelectionRef.current) return; + // Nothing to manage for players that neither lock nor track recordings + // (Mark Verses, Transcribe, the generic player) — leave their regions alone. + if (!locked && !recorded) return; const sorted = sortedRegions(); const last = sorted.length - 1; sorted.forEach((r, i) => { - // A side is draggable only when neither this segment nor the neighbor on - // that side is recorded. - const self = recorded(i); - const startActive = !self && (i === 0 || !recorded(i - 1)); - const endActive = !self && (i === last || !recorded(i + 1)); - // Keep handles visible; gate drag by side with resizeStart/resizeEnd. + let startActive: boolean; + let endActive: boolean; + if (locked) { + // Recording in progress: nothing moves. + startActive = false; + endActive = false; + } else { + // A side is draggable only when neither this segment nor the neighbour + // on that side is recorded. Outer edges have no neighbour. + const self = recorded!(i); + startActive = !self && (i === 0 || !recorded!(i - 1)); + endActive = !self && (i === last || !recorded!(i + 1)); + } + // Keep the handle rendered; gate drag by side with resizeStart/resizeEnd. r.setOptions({ resize: true, resizeStart: startActive, @@ -594,8 +583,12 @@ export function useWaveSurferRegions( regionsPlugin.on('region-created', function (r: Region) { if (isMarker(r)) return; r.drag = singleRegionRef.current; - // If region is created while lock is active, freeze it immediately. - if (lockSegmentSelectionRef.current) freezeRegionDrag(r); + // A region born while the recording lock is up must not be draggable + // even for a moment; freeze both sides now (handle stays visible). The + // reactive pass / next load sets the full recorded-aware state. + if (lockSegmentSelectionRef.current) { + r.setOptions({ resize: true, resizeStart: false, resizeEnd: false }); + } // Round region start and end to 5 decimal places because the seek uses 5 decimal places r.start = roundToFiveDecimals(r.start); @@ -796,7 +789,7 @@ export function useWaveSurferRegions( }); } // Apply recorded-boundary locks for existing segments (TT-7666). - applyRecordedResizeLocks(); + applyBoundaryEditability(); } }; @@ -1087,7 +1080,7 @@ export function useWaveSurferRegions( }); setPrevNext(regarray.map((r: any) => r.id)); // Re-apply recorded-boundary locks after loading regions (TT-7666). - applyRecordedResizeLocks(); + applyBoundaryEditability(); onRegion(regarray.length, newRegions); onRegionGoTo(regarray[defaultRegionIndex]?.start ?? 0); loadingRef.current = false; From 8cada599f1ece849d5d8510f20265b5c897db8de Mon Sep 17 00:00:00 2001 From: Noel Chou Date: Fri, 4 Sep 2026 10:01:38 -0400 Subject: [PATCH 14/27] TT-7666 +/- report handled only when the edit actually happened 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) --- src/renderer/src/components/WSAudioPlayerSegment.tsx | 7 +++++-- 1 file changed, 5 insertions(+), 2 deletions(-) diff --git a/src/renderer/src/components/WSAudioPlayerSegment.tsx b/src/renderer/src/components/WSAudioPlayerSegment.tsx index 685c7088a..d2c3e6a9e 100644 --- a/src/renderer/src/components/WSAudioPlayerSegment.tsx +++ b/src/renderer/src/components/WSAudioPlayerSegment.tsx @@ -130,7 +130,9 @@ function WSAudioPlayerSegment(props: IProps) { const result = wsAddRegion(); if (result && onSplit) onSplit(result); if (setBusy) setBusy(false); - return true; + // Report handled only when a divider was actually added — a refused split + // (recorded segment, or recording in progress) did nothing (TT-7666). + return !!result; }; const handleRemoveNextSplit = () => { if (!readyRef.current) return false; @@ -139,7 +141,8 @@ function WSAudioPlayerSegment(props: IProps) { const result = wsRemoveSplitRegion(); if (result && onSplit) onSplit(result); if (setBusy) setBusy(false); - return true; + // Report handled only when a boundary was actually removed (TT-7666). + return !!result; }; const handleSegParamChange = ( params: IRegionParams, From 984a2d51073868f76c81e32ab25cda52041116b4 Mon Sep 17 00:00:00 2001 From: Noel Chou Date: Fri, 4 Sep 2026 10:41:20 -0400 Subject: [PATCH 15/27] TT-7666 perf: don't re-sort regionBounds in the +/- disable helpers 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) --- .../src/components/segmentBoundaryLocks.ts | 23 +++++++++++-------- 1 file changed, 13 insertions(+), 10 deletions(-) diff --git a/src/renderer/src/components/segmentBoundaryLocks.ts b/src/renderer/src/components/segmentBoundaryLocks.ts index ff106c791..d86a39d40 100644 --- a/src/renderer/src/components/segmentBoundaryLocks.ts +++ b/src/renderer/src/components/segmentBoundaryLocks.ts @@ -3,19 +3,22 @@ import { IRegion } from '../crud/useWavesurferRegions'; /** * Helpers for disabling +/- when a recorded segment would be changed * (TT-7666). Kept separate for easier unit testing. + * + * `regions` must already be sorted by start — callers pass the player's + * `regionBounds` (built via getSortedRegions). These run on every `progress` + * update, so they iterate the array directly rather than cloning and sorting. */ -/** Sorted index of the segment at the playhead, or -1. */ +/** Sorted index of the segment at the playhead, or -1. Assumes sorted input. */ 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; + for (let i = 0; i < regions.length; i++) { + const isLast = i === regions.length - 1; if ( - progressSec >= sorted[i].start && - (isLast ? progressSec <= sorted[i].end : progressSec < sorted[i].end) + progressSec >= regions[i].start && + (isLast ? progressSec <= regions[i].end : progressSec < regions[i].end) ) { return i; } @@ -34,7 +37,8 @@ export function isAddBlockedByRecording( return idx >= 0 && isSegmentRecorded(idx); } -/** Block Remove when either side of the merged boundary is recorded (TT-7666). */ +/** Block Remove when either side of the merged boundary is recorded (TT-7666). + * Assumes sorted input. */ export function isRemoveBlockedByRecording( progressSec: number, regions: IRegion[], @@ -42,9 +46,8 @@ export function isRemoveBlockedByRecording( isSegmentRecorded?: (index: number) => boolean ): boolean { 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) { + for (let i = 0; i < regions.length - 1; i++) { + if (Math.abs(progressSec - regions[i].end) <= tol) { return isSegmentRecorded(i) || isSegmentRecorded(i + 1); } } From acbbfc1861aff4e1e80a1470e4f0edb86ef8ee63 Mon Sep 17 00:00:00 2001 From: Noel Chou Date: Fri, 4 Sep 2026 13:12:52 -0400 Subject: [PATCH 16/27] TT-7666 clear the segment-edit undo history when a take is recorded MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Devin: undo was not gated by recorded state, so splitting/combining a clause, recording a sub-clause, then undoing restored the pre-edit boundaries — the saved take no longer matched a segment (worse in Phrase BT, whose player undo is multi-level). Per the product call, recording a take fixes the boundaries around it, so record-start now clears both the one-level Combine undo and the multi-level segment undo stack: there is simply no history to restore across a take. Test: undo armed by a Combine is gone once recording starts. (Devin's other new finding — an in-flight boundary drag racing a mid-drag lock activation — is left as-is: it does not point to a clean simplification, the handle freeze already prevents the drag at the source, and the backstop prevents persistence; the residual is a cosmetic, self-healing bound only reachable by a single-pointer-impossible gesture.) Co-Authored-By: Claude Opus 4.8 (1M context) --- .../PassageDetailCarefulSpeech.test.tsx | 21 +++++++++++++++++++ .../PassageDetailGuidedPhraseRecord.tsx | 5 +++++ 2 files changed, 26 insertions(+) diff --git a/src/renderer/src/components/PassageDetail/PassageDetailCarefulSpeech.test.tsx b/src/renderer/src/components/PassageDetail/PassageDetailCarefulSpeech.test.tsx index 9f75b98d1..9d5613dfa 100644 --- a/src/renderer/src/components/PassageDetail/PassageDetailCarefulSpeech.test.tsx +++ b/src/renderer/src/components/PassageDetail/PassageDetailCarefulSpeech.test.tsx @@ -798,3 +798,24 @@ describe('PassageDetailCarefulSpeech — recorded-state consistency across guard expect(controlsProps?.canCombineWithNext).toBe(true); }); }); + +describe('PassageDetailCarefulSpeech — undo history cleared at record start (TT-7666)', () => { + it('drops the segment-edit undo once a take is recorded', async () => { + mockCompleted = new Set([0, 1]); // clause 2 is the current unrecorded one + await mountAndSettle(); + await firePlaybackEnd(2); + + // Arm undo with a boundary edit (Combine). + await act(async () => { + await (controlsProps?.onCombineWithNext as () => Promise)(); + }); + expect(controlsProps?.showUndoCombine).toBe(true); + + // Recording fixes the boundaries; the undo history must be gone so it can + // never restore boundaries the take no longer matches. + await act(async () => { + (controlsProps?.onRecording as (active: boolean) => void)(true); + }); + expect(controlsProps?.showUndoCombine).toBe(false); + }); +}); diff --git a/src/renderer/src/components/PassageDetail/PassageDetailGuidedPhraseRecord.tsx b/src/renderer/src/components/PassageDetail/PassageDetailGuidedPhraseRecord.tsx index bea3eb57d..f8d2a2db6 100644 --- a/src/renderer/src/components/PassageDetail/PassageDetailGuidedPhraseRecord.tsx +++ b/src/renderer/src/components/PassageDetail/PassageDetailGuidedPhraseRecord.tsx @@ -2074,6 +2074,11 @@ export function PassageDetailGuidedPhraseRecord({ // TT-7552: a deliberate take cancels the post-park overshoot swallow // so tapping the next segment is treated as real navigation. pendingOvershootSwallowRef.current = false; + // Recording a take fixes the boundaries around it, so drop the + // segment-edit undo history — undoing a prior split/combine after a + // take would restore boundaries the take no longer matches (TT-7666). + clearSegmentUndo(); + setCombineUndo(null); setRecording(true); setPhase('recording'); return; From 964d50ec229214d2aa0614d9b00a196fb2fafe35 Mon Sep 17 00:00:00 2001 From: Noel Chou Date: Fri, 4 Sep 2026 15:58:03 -0400 Subject: [PATCH 17/27] reword comments --- .../PassageDetailGuidedPhraseRecord.tsx | 220 ++++-------------- .../src/crud/useWavesurferRegions.tsx | 64 ++--- 2 files changed, 67 insertions(+), 217 deletions(-) diff --git a/src/renderer/src/components/PassageDetail/PassageDetailGuidedPhraseRecord.tsx b/src/renderer/src/components/PassageDetail/PassageDetailGuidedPhraseRecord.tsx index f8d2a2db6..85f9aba8a 100644 --- a/src/renderer/src/components/PassageDetail/PassageDetailGuidedPhraseRecord.tsx +++ b/src/renderer/src/components/PassageDetail/PassageDetailGuidedPhraseRecord.tsx @@ -206,12 +206,7 @@ export function PassageDetailGuidedPhraseRecord({ const initialPositionDoneRef = useRef(false); const suppressClauseAutoPlayRef = useRef(0); const playClauseInFlightRef = useRef(false); - /** - * When playback of the current clause last began - whether this component - * started it or the user pressed Play. See SPURIOUS_STOP_WINDOW_MS: a stale - * timestamp would make the seek-suppression window compare against an old - * time and mark a clause played before it had been. - */ + /** Last clause-play start time, used by SPURIOUS_STOP_WINDOW_MS filtering. */ const clausePlaybackStartedAtRef = useRef(0); const skipBeforePlayRef = useRef(false); const entryPauseDoneRef = useRef(false); @@ -238,29 +233,19 @@ export function PassageDetailGuidedPhraseRecord({ const [savingRecording, setSavingRecording] = useState(false); // Mirrors saveRejectedRef for render: shows the failure message + Retry. const [saveRejected, setSaveRejected] = useState(false); - // Mirror of savingRecording, set synchronously wherever the state is toggled. - // The clause-boundary guards read this ref rather than the state value so a - // save that begins before React commits the disabling render is still seen by - // an in-flight callback closure (state would be captured stale). See the - // savingRecording toggles below (TT-7427). + // Sync mirror for callbacks so boundary guards see save-start immediately, + // even before React commits the render update (TT-7427). const savingRecordingRef = useRef(false); const [recordingPassStarted, setRecordingPassStarted] = useState(false); - // Mirror of recordingPassStarted set synchronously at the call sites below. - // region-out can fire before React commits the state-update render, leaving - // the handler's closure (and handleRegionPlayEndRef) on the stale false - // value; reading this ref makes the recording/listen branch decision reflect - // intent immediately rather than waiting for a render (TT-7360). + // Sync mirror for region-out callbacks so pass-branching does not read stale + // state during render lag (TT-7360). const recordingPassStartedRef = useRef(false); /** Mirrors context recording for segment-lock checks once capture is active. */ const recordingActiveRef = useRef(false); - // Set true when we park after an auto-play. The playback overshoot into the - // next clause (or a recorder-mount-induced region-in) advances by exactly one - // clause; this lets the recording effect swallow that single +1 advance while - // still treating any non-adjacent jump as a genuine user tap (TT-7360). + // Armed after auto-play park to ignore one spurious +1 clause jump + // (overshoot/region-in) while still allowing real navigation (TT-7360). const pendingOvershootSwallowRef = useRef(false); - /** Indices saved this session whose rowData may not have caught up yet (TT-7552). - * Read synchronously (coloring) via the ref; the version below makes changes - * to it reactive so the recorded-aware guards recompute (TT-7666). */ + /** Session-local saved indices before rowData catches up (TT-7552, TT-7666). */ const optimisticCompletedRef = useRef>(new Set()); const [optimisticVersion, setOptimisticVersion] = useState(0); const bumpOptimistic = useCallback( @@ -286,13 +271,7 @@ export function PassageDetailGuidedPhraseRecord({ bumpOptimistic(); }, [bumpOptimistic]); const currentIndexRef = useRef(0); - /** - * Clause and region for the active take (TT-7437). - * - * We lock this when recording starts and keep it until the take is saved or - * discarded. Save-time values (sourceSegments, filename postfix, completion - * color) must come from this latched target, not from live selection. - */ + /** Latched clause/region for the active take until save or discard (TT-7437). */ const [recordingTarget, setRecordingTarget] = useState< { index: number; region: IRegion } | undefined >(undefined); @@ -796,17 +775,8 @@ export function PassageDetailGuidedPhraseRecord({ setPhase('playing'); const seek = region.start > 0 ? region.start + CLAUSE_BOUNDARY_THRESHOLD_SEC : 0; - // How long this clause will take, so Record can be withheld for exactly - // that long. See recordBlocked for why this is timed rather than driven - // by an event. - // Always 1 as things stand: the speed control is only rendered when - // PassageDetailPlayer is given allowSpeed or allowZoomAndSpeed, and this - // step passes neither, so nothing here can change the rate. Dividing by - // it is for whenever that is turned on - at 0.25x a clause takes four - // times as long, and withholding Record for the unscaled span would - // release it three quarters of the way through the audio. Untested at - // any rate other than 1 for the same reason: there is no way to reach - // one from this step yet. + // Compute expected playback span so Record stays disabled for the full + // clause duration. Use rate for future speed-enabled flows. const rate = ctrl.getPlaybackRate?.() || 1; setRecordBlockedUntil( Date.now() + @@ -876,21 +846,10 @@ export function PassageDetailGuidedPhraseRecord({ (playingNow: boolean) => { if (playingNow) { setHighlightPlayButton(false); - // Playback began, whatever started it. Time the window from here, so a - // stop that this start provokes is discarded below instead of being read - // as the clause having finished. + // Reset stop-filter timing at every playback start. clausePlaybackStartedAtRef.current = Date.now(); - // Withhold Record for what is left of the clause. playCurrentClause sets - // this too, for the step's own playback, but a user replaying a clause - // they have already heard never goes through it - and by then the clause - // counts as heard, so Record is operable and could be pressed over the - // reference audio, which is the very thing withholding it prevents - // (Devin). Measured from the playhead, since a replay can start part way - // in. - // - // Set here rather than in beforePlay: the player awaits that hook, and - // setting state inside it re-renders mid-start and the play never - // happens at all. Here the audio is already running. + // Also gate Record for user-triggered replays from current playhead. + // Set here (not beforePlay) so playback start is not interrupted. const region = clauseRegions[currentIndex]; const ctrl = playerControlsRef.current; if (region && ctrl) { @@ -915,32 +874,16 @@ export function PassageDetailGuidedPhraseRecord({ // Capturing or saving a take stops the source audio, and that stop is not // the clause being heard. Defensive: if (recordingActiveRef.current || savingRecordingRef.current) return; - // Starting a clause seeks the playhead to its start, and that seek reports - // a stop of its own before anything has been heard. This can be - // differentiated from a real stop (user pause) by how long playback had - // been running. If under SPURIOUS_STOP_WINDOW_MS it is the seek, ignore it. + // Ignore synthetic stop from start seek if it lands inside the spurious + // stop window. if ( Date.now() - clausePlaybackStartedAtRef.current < SPURIOUS_STOP_WINDOW_MS ) { return; } - // A real stop - the user pausing - means the rest of the clause is not - // coming, so stop withholding Record for it (#529's behaviour, kept). - // - // A pause inside the window above does not get here, so Record stays - // withheld for the remainder of the clause's span even though the audio - // has stopped (Devin). Deliberate: inside that window a stop cannot be - // told apart from the seek that starts the clause, and clearing on the - // seek would reinstate the defect this all exists to fix. The wait is - // bounded by the clause length, and it is the same limitation #529 already - // has for currentClausePlayed - a pause that quick does not mark the - // clause heard either. - // - // Reaching it means pausing within 250ms of a clause starting, which - // Noel's call is unlikely in practice and near enough impossible without a - // touchscreen: the step starts the clause itself, so it needs the pointer - // already over Play and a press inside that window. + // Real user pause: release Record block and mark clause as heard. + // Very-early pauses are treated like start-seek noise by design. setRecordBlockedUntil(0); setCurrentClausePlayed(true); setPhase((p) => @@ -984,12 +927,8 @@ export function PassageDetailGuidedPhraseRecord({ }, [mediafileId]); useEffect(() => { - // StrictMode double-invokes effects on mount (setup → cleanup → setup) and - // refs persist across that re-run. Without this guard the second invocation - // re-resets recordingPassStarted to false and clears initialPositionDoneRef - // mid-entry, clobbering an already-started recording pass and dropping the - // user back into the listen pass (TT-7360). Only reset once per actual - // mediafile change. + // StrictMode mount re-run can reset pass state mid-entry. Guard so reset + // runs once per real mediafile change (TT-7360). if (lastResetMediafileRef.current === mediafileId) return; lastResetMediafileRef.current = mediafileId; resetForMediafile(mediafileId); @@ -1020,10 +959,8 @@ export function PassageDetailGuidedPhraseRecord({ setEntryPositioned(false); suppressClauseAutoPlayRef.current = 0; setHighlightPlayButton(false); - // Gate only on the stable mediafileId string. resetForMediafile's identity - // changes whenever the mediafile record is updated (e.g. persisting combined - // clause segments), which would otherwise re-fire this reset and drop the - // user from the recording pass back into the listen pass (TT-7360). + // Depend only on stable mediafileId. resetForMediafile identity changes + // during media updates and would incorrectly reset pass state (TT-7360). // eslint-disable-next-line react-hooks/exhaustive-deps }, [mediafileId]); @@ -1090,9 +1027,7 @@ export function PassageDetailGuidedPhraseRecord({ ); const firstIdx = firstIncompleteClauseIndex(clauseRegions, completed); if (firstIdx >= clauseRegions.length) { - // All clauses are recorded. Enter recording (review) mode positioned on - // the first clause so the user can replay both the original and the - // careful-speech take per clause. Step completion syncs via effect. + // All clauses recorded: enter review mode on first clause. setRecordingPassStarted(true); recordingPassStartedRef.current = true; setShowRecorder(true); @@ -1233,11 +1168,8 @@ export function PassageDetailGuidedPhraseRecord({ setCurrentIndex(idx); setCurrentClausePlayed(true); - // Parking after an auto-play. A spurious +1 segment change usually - // follows — playback overshoot into the next clause, or the recorder - // mounting once allowRecord turns true — which the recording effect would - // otherwise read as a user tap and auto-play. Arm the overshoot swallow so - // that single adjacent advance is ignored while we stay parked (TT-7360). + // After auto-play park, ignore one spurious +1 region change caused by + // overshoot or recorder mount (TT-7360). pendingOvershootSwallowRef.current = true; if (phase === 'recording') return; setPhase('recordReady'); @@ -1254,23 +1186,15 @@ export function PassageDetailGuidedPhraseRecord({ ] ); - // WaveSurfer registers region-out once on audio ready, capturing a snapshot - // of this callback. Start Recording changes recordingPassStarted without - // reloading the audio, which would leave the listener on a stale listen-pass - // closure. Stable wrapper + ref keeps it current. See docs/adr/0006. + // region-out listener is registered once; route through ref to avoid stale + // closure when recording pass changes without audio reload (ADR 0006). const handleRegionPlayEndRef = useRef(handleRegionPlayEnd); handleRegionPlayEndRef.current = handleRegionPlayEnd; const onSegmentPlaybackEnd = useCallback((region: IRegion) => { 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. - */ + /** User click is intentional navigation, so never treat it as overshoot. */ const handleSegmentClick = useCallback(() => { pendingOvershootSwallowRef.current = false; }, []); @@ -1294,17 +1218,8 @@ export function PassageDetailGuidedPhraseRecord({ const indexChanged = idx !== currentIndex; if (pendingOvershootSwallowRef.current && idx === currentIndex + 1) { - // A +1 segment change arriving just after an auto-play park is not the - // user moving: either playback overshot into the next clause, or the - // recorder mounted and reported a region-in there. Stay on the parked - // clause and play nothing. A change further away than +1 is a real tap, - // and falls through to the branches below. - // - // This has to be decided before the completed-clause branch below. - // Otherwise, when the clause the overshoot lands on already has a take - - // record 1, arrow forward to 3, record 3, arrow back to 2 - that branch - // takes the overshoot for a move onto clause 3 and selects it, leaving - // clause 2 pending under a user who was waiting to record it (TT-7621). + // Swallow one adjacent +1 change right after auto-play park. It is + // usually overshoot/region-in noise, not user navigation (TT-7621). pendingOvershootSwallowRef.current = false; if (playerControlsRef.current?.isPlaying?.()) { playerControlsRef.current.setPlay(false); @@ -1352,20 +1267,9 @@ export function PassageDetailGuidedPhraseRecord({ setCurrentClausePlayed(false); setPhase((p) => (p === 'recording' ? 'recording' : 'readyToRecord')); void playCurrentClause(idx); - // Neither dep is read in the body — the segment itself comes from the ref - // behind getCurrentSegment() — so both are here purely as change signals. - // - // currentSegmentSeq is the one that tells us the selection moved. The - // index's numbering is not agreed between writers (the waveform writes - // 1-based, this component 0-based), so a genuine move can arrive carrying - // the number the previous writer used. Clicking segment 2 right after - // recording segment 3 does exactly that (waveform 1+1 vs step 2) — the - // effect never re-ran, the step stayed on segment 3, and the next take was - // filed there. - // - // currentSegmentIndex stays because the seq does not fully cover it: - // selecting another row resets the index to 0 without going through - // setCurrentSegment, so no seq bump accompanies that one. + // Both deps are change signals for getCurrentSegment(). Keep + // currentSegmentSeq for reliable move detection across index conventions, + // and keep currentSegmentIndex for row-change resets that skip seq bumps. }, [ currentSegmentIndex, currentSegmentSeq, @@ -1779,12 +1683,8 @@ export function PassageDetailGuidedPhraseRecord({ setCurrentClausePlayed(false); setPhase('readyToRecord'); setResetMedia(true); - // playCurrentClause plays clause `next` once. When it ends, - // handleRegionPlayEnd parks us on `next` (recordReady) and arms the - // overshoot swallow, so the region-in into next+1 is ignored by the - // recording effect rather than advancing the clause. This replaces the old - // duration-based setTimeout that re-asserted `next` after playback — see - // TT-7360. + // Play next once; region-end handler parks on it and arms overshoot swallow + // so next+1 region-in does not auto-advance (TT-7360). await playCurrentClause(next); }, [ clauseRegions, @@ -1798,12 +1698,9 @@ export function PassageDetailGuidedPhraseRecord({ const afterUploadCb = useCallback( async (mediaId: string | undefined) => { - // Color green immediately; rowData/forceRefresh often lag the upload - // (TT-7552). Only on a real upload though — a terminal failure still calls - // us, with no mediaId, and painting that green tells the user their take - // was stored when it was not (TT-7583). - // Mark completion on the clause where recording started (TT-7437), - // not on the current live selection. + // Mark optimistic completion immediately after real upload (TT-7552), + // and always apply it to the latched recording-start clause (TT-7437). + // No mediaId means upload failed; do not show optimistic success (TT-7583). const takenIndex = recordingTargetRef.current?.index ?? currentIndexRef.current; if (mediaId) { @@ -1815,9 +1712,8 @@ export function PassageDetailGuidedPhraseRecord({ // Keep the latch on failed upload so Retry files to the same clause // even if selection moved (TT-7583). } - // Stays 'recorded' either way: the take still exists, it just is not - // stored. That keeps Record disabled and the clear button available, so - // discarding the take is the deliberate way back to recording (TT-7583). + // Keep 'recorded' on success or failure so user can clear/retry this take + // instead of starting a new one accidentally (TT-7583). setPhase('recorded'); savingRecordingRef.current = false; setSavingRecording(false); @@ -1835,13 +1731,11 @@ export function PassageDetailGuidedPhraseRecord({ ); const handleClearRecording = useCallback(async () => { - // Deleting the take retires the failed save with it, so the message and the - // latch must both go (TT-7583). + // Clearing the take also clears failed-save state and latch (TT-7583). saveRejectedRef.current = false; setSaveRejected(false); - // A take whose upload failed has no mediafile to remove, but it is still - // sitting unsaved in the recorder — discarding it is the whole point of the - // button in that state, so only the removal is conditional (TT-7583). + // Failed uploads may have no mediafile record, but Clear still discards the + // local take (TT-7583). const mediaId = recordingRow?.mediafile?.id; if (mediaId) { await memory.update((t) => @@ -1878,31 +1772,13 @@ export function PassageDetailGuidedPhraseRecord({ !completedIndices.has(currentIndex); /** - * Withhold Record for as long as the clause will take to play. - * - * Recording over the reference audio is what the listen-then-record flow - * exists to prevent, and the step's phase flags do not enforce it: the seek - * that starts a clause emits a region-out indistinguishable from the one that - * ends it, so handleRegionPlayEnd parks and marks the clause heard about 60ms - * in, leaving Record operable for the rest of it. - * - * Two event-based fixes were tried and reverted. Withholding the park strands - * the step, because the navigation flows are built on it and the genuine end - * is not reliably reported. Gating on the player's own playing state flickers, - * because starting a clause pauses and resumes it - and that blip turns out to - * be load-bearing: it is what makes the end-of-region event arrive at all, so - * removing it strands the step too. See ADR 0011. + * Disable only the Record button until expected clause playback time expires. * - * So don't infer it from events. The clause span and the playback rate are - * both known when playback starts, so how long the audio will run is known - * too. This degrades safely in every direction: if playback is cut short - * Record returns a little late, if it is a sliver Record returns almost at - * once, and nothing can leave it withheld indefinitely. + * We use time-based gating because early region-out events from start seeks + * can look like real playback end and re-enable Record too soon (ADR 0011). * - * Deliberately not folded into allowRecord: that prop is capability, and - * useWavRecorder releases the capture stream when it goes false, so a take - * could not start until the microphone had been re-acquired. This disables the - * button only, and changes no step state. + * This is intentionally separate from allowRecord so recorder capability and + * mic lifecycle state are not toggled by this temporary UI gate. */ useEffect(() => { const remaining = recordBlockedUntil - Date.now(); diff --git a/src/renderer/src/crud/useWavesurferRegions.tsx b/src/renderer/src/crud/useWavesurferRegions.tsx index 52d9c3d5b..c29a6d6e4 100644 --- a/src/renderer/src/crud/useWavesurferRegions.tsx +++ b/src/renderer/src/crud/useWavesurferRegions.tsx @@ -126,11 +126,8 @@ export function useWaveSurferRegions( lockSegmentSelection?: boolean, getDecodedBuffer?: () => AudioBuffer | undefined, /** - * Suppress drag-to-create-region (the red loop region) even in single-region - * mode. Read once, where setupRegions configures the regions plugin on the - * WaveSurfer 'ready' event — it is not reactive, so toggling it afterwards has - * no effect until the next load. Pass a value that is constant for the - * lifetime of the player. + * Disable drag-to-create-region (red loop region) in single-region mode. + * Read once during setupRegions on WaveSurfer ready; not reactive. */ disableDragSelection?: boolean, /** @@ -200,12 +197,8 @@ export function useWaveSurferRegions( applyRegionColorRef.current = applyRegionColor; }, [applyRegionColor]); - // One reactive pass owns which boundaries can be dragged. It re-runs whenever - // the recording lock (TT-7437) or the recorded-segment set (TT-7666) changes, - // so freezing and un-freezing — e.g. a boundary coming back after its - // recording is deleted — happen the same way, on the same signal, as the - // Split/Combine and +/- guards that read the same state. Handles are never - // removed; only their per-side drag is toggled (see applyBoundaryEditability). + // Recompute boundary dragability whenever lock state or recorded-set changes + // so all boundary guards react on the same signal (TT-7437, TT-7666). useEffect(() => { lockSegmentSelectionRef.current = lockSegmentSelection ?? false; applyBoundaryEditability(); @@ -270,18 +263,14 @@ export function useWaveSurferRegions( }; /** - * The single source of truth for which boundaries can be dragged. A side is - * draggable unless the recording lock is up (TT-7437) or the segment — or its - * neighbour across that side — is recorded (TT-7666). Handles are always left - * visible (resize: true); only the per-side drag (resizeStart/resizeEnd) and - * the cursor change, so a boundary marker never disappears. Idempotent, so it - * is safe to run on every color pass, region load, and lock/recorded change. + * Source of truth for boundary dragability. + * Drag is blocked by recording lock (TT-7437) and by recorded boundaries + * (TT-7666). Handles stay visible; only side drag flags/cursor change. */ const applyBoundaryEditability = () => { const locked = lockSegmentSelectionRef.current; const recorded = isSegmentRecordedRef.current; - // Nothing to manage for players that neither lock nor track recordings - // (Mark Verses, Transcribe, the generic player) — leave their regions alone. + // Players without lock/recorded checks keep default region behavior. if (!locked && !recorded) return; const sorted = sortedRegions(); const last = sorted.length - 1; @@ -293,13 +282,13 @@ export function useWaveSurferRegions( startActive = false; endActive = false; } else { - // A side is draggable only when neither this segment nor the neighbour - // on that side is recorded. Outer edges have no neighbour. + // Side drag is allowed only if neither this segment nor that side's + // neighbor is recorded. Outer edges have no neighbor. const self = recorded!(i); startActive = !self && (i === 0 || !recorded!(i - 1)); endActive = !self && (i === last || !recorded!(i + 1)); } - // Keep the handle rendered; gate drag by side with resizeStart/resizeEnd. + // Keep handles visible; gate side drag via resizeStart/resizeEnd. r.setOptions({ resize: true, resizeStart: startActive, @@ -507,11 +496,8 @@ export function useWaveSurferRegions( } }; const wsPlayRegion = (r: IRegion, startAtCurrent: boolean = false) => { - // updatingRef suppresses the shared-boundary clamp while *we* are moving - // region bounds; it must be released on every exit path. Players with - // forceRegionOnly (Phrase Back Translate, Careful Speech) route every play - // through here, so a latched flag left the clamp off for the rest of the - // session and dragged boundaries overlapped or disconnected (TT-7625). + // While we move bounds programmatically, suppress boundary clamp and always + // release the flag on exit to avoid persistent clamp-off state (TT-7625). updatingRef.current = true; try { let reg = findRegion(r.start, true); @@ -583,9 +569,8 @@ export function useWaveSurferRegions( regionsPlugin.on('region-created', function (r: Region) { if (isMarker(r)) return; r.drag = singleRegionRef.current; - // A region born while the recording lock is up must not be draggable - // even for a moment; freeze both sides now (handle stays visible). The - // reactive pass / next load sets the full recorded-aware state. + // New region during recording lock: freeze both sides immediately. + // Reactive pass/load will apply full recorded-aware state. if (lockSegmentSelectionRef.current) { r.setOptions({ resize: true, resizeStart: false, resizeEnd: false }); } @@ -723,10 +708,8 @@ export function useWaveSurferRegions( }); regionsPlugin.on('region-out', function (r: Region) { if (isMarker(r)) return; - // If this region was just truncated by a split (matched by id, or by - // end-time within the autosave-replacement window), ignore region-out - // so playback continues into the new right-side region without - // stopping or seeking back to the start. + // Ignore region-out for just-truncated split region (id or end-time + // match) so playback continues into the new right region. const matchesTruncatedId = r.id === splitTruncatedIdRef.current; const matchesTruncatedEnd = splitTruncatedEndRef.current !== undefined && @@ -875,17 +858,8 @@ export function useWaveSurferRegions( }; /** - * Keep resized regions non-overlapping. In multi-region (Mark Verses, - * Careful Speech, Phrase Back Translate) mode the end of one region is - * always the start of the next, so a boundary is shared by two regions. - * When the user drags one boundary we: - * - clamp it so it can't cross the neighbor's far boundary (no overlap) — - * the first/last region's outer edge stays pinned to 0 / duration; and - * - shift the single adjacent neighbor's shared boundary to follow, so the - * two regions stay flush. - * `side` is provided by the regions plugin ('start' | 'end') and tells us - * which boundary is moving; when absent (defensive) we constrain both. - * Returns the boundary time the drag settled on for playhead follow. + * Keep resized regions non-overlapping and contiguous by clamping the moved + * boundary and updating the adjacent shared boundary. Returns final boundary. */ const constrainResizedRegion = (r: Region, side?: 'start' | 'end') => { const prev = findPrevRegion(r) as Region | undefined; From 8ed70977ed36e1ca5d86b169e0e55274a992253b Mon Sep 17 00:00:00 2001 From: Noel Chou Date: Fri, 4 Sep 2026 16:36:23 -0400 Subject: [PATCH 18/27] Rename recordedForTools -> recordedClauseIndicesForTools for clarity The variable is a Set of clause indices with recordings; the new name states the type (indices, not a boolean) and its purpose (the split/combine tool guards' view, which includes optimistic completions). Co-Authored-By: Claude Opus 4.8 (1M context) --- .../PassageDetailGuidedPhraseRecord.tsx | 14 +++++++------- 1 file changed, 7 insertions(+), 7 deletions(-) diff --git a/src/renderer/src/components/PassageDetail/PassageDetailGuidedPhraseRecord.tsx b/src/renderer/src/components/PassageDetail/PassageDetailGuidedPhraseRecord.tsx index 85f9aba8a..7eb34e2db 100644 --- a/src/renderer/src/components/PassageDetail/PassageDetailGuidedPhraseRecord.tsx +++ b/src/renderer/src/components/PassageDetail/PassageDetailGuidedPhraseRecord.tsx @@ -500,7 +500,7 @@ export function PassageDetailGuidedPhraseRecord({ /** completedIndices plus the optimistic just-saved set — the single recorded * view every boundary-editing guard uses, so they agree in the window before * rowData catches up (TT-7666). */ - const recordedForTools = useMemo( + const recordedClauseIndicesForTools = useMemo( () => new Set([...completedIndices, ...optimisticCompletedRef.current]), // eslint-disable-next-line react-hooks/exhaustive-deps [completedIndices, optimisticVersion] @@ -1458,7 +1458,7 @@ export function PassageDetailGuidedPhraseRecord({ phraseSegParams ); if ( - !canSplitClause(currentIndex, clauseRegions, recordedForTools, splitPoint) + !canSplitClause(currentIndex, clauseRegions, recordedClauseIndicesForTools, splitPoint) ) { return; } @@ -1482,7 +1482,7 @@ export function PassageDetailGuidedPhraseRecord({ }, [ currentIndex, clauseRegions, - recordedForTools, + recordedClauseIndicesForTools, clauseSegString, phraseSegParams, setClauseSegString, @@ -1496,7 +1496,7 @@ export function PassageDetailGuidedPhraseRecord({ const handleCombineWithNext = useCallback(async () => { if (savingRecordingRef.current) return; - if (!canCombineWithNext(currentIndex, clauseRegions, recordedForTools)) { + if (!canCombineWithNext(currentIndex, clauseRegions, recordedClauseIndicesForTools)) { return; } const updated = mergeClauseWithNext(clauseRegions, currentIndex); @@ -1515,7 +1515,7 @@ export function PassageDetailGuidedPhraseRecord({ }, [ currentIndex, clauseRegions, - recordedForTools, + recordedClauseIndicesForTools, clauseSegString, phraseSegParams, setClauseSegString, @@ -1896,13 +1896,13 @@ export function PassageDetailGuidedPhraseRecord({ canSplitClause={canSplitClause( currentIndex, clauseRegions, - recordedForTools, + recordedClauseIndicesForTools, currentClauseSplitPoint )} canCombineWithNext={canCombineWithNext( currentIndex, clauseRegions, - recordedForTools + recordedClauseIndicesForTools )} showUndoCombine={ combineUndo !== null && !config.multiLevelSegmentUndo From cbc2c53edf1d0acea0a99eec9e84d7d93cc6f29c Mon Sep 17 00:00:00 2001 From: Noel Chou Date: Fri, 4 Sep 2026 17:03:12 -0400 Subject: [PATCH 19/27] little rename --- src/renderer/src/components/segmentBoundaryLocks.ts | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/src/renderer/src/components/segmentBoundaryLocks.ts b/src/renderer/src/components/segmentBoundaryLocks.ts index d86a39d40..e377cb28a 100644 --- a/src/renderer/src/components/segmentBoundaryLocks.ts +++ b/src/renderer/src/components/segmentBoundaryLocks.ts @@ -10,7 +10,7 @@ import { IRegion } from '../crud/useWavesurferRegions'; */ /** Sorted index of the segment at the playhead, or -1. Assumes sorted input. */ -export function segmentIndexAtProgress( +export function segmentIndexAtPlayhead( progressSec: number, regions: IRegion[] ): number { @@ -33,7 +33,7 @@ export function isAddBlockedByRecording( isSegmentRecorded?: (index: number) => boolean ): boolean { if (!isSegmentRecorded) return false; - const idx = segmentIndexAtProgress(progressSec, regions); + const idx = segmentIndexAtPlayhead(progressSec, regions); return idx >= 0 && isSegmentRecorded(idx); } From 76c27efd1b1f1c8efe982de2dec4f36d3fe6ef4e Mon Sep 17 00:00:00 2001 From: Noel Chou Date: Fri, 4 Sep 2026 18:59:00 -0400 Subject: [PATCH 20/27] TT-7437 lock the clause of a recorded-but-unsaved take MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit A take whose upload failed is kept latched to its clause and stays playable via the recorder for Retry, but nothing marked that clause recorded: phase is 'recorded' (so segmentSelectionLocked is off) and neither completedIndices nor the optimistic set holds it. That re-enabled boundary drag, +/- and Split/Combine on a clause that still owns a take, so the user could reshape the segment and a Retry would then file the take against the altered boundaries — the exact class of bug this ticket fixes. Derive a pendingTakeIndex (the latched target while phase is 'recorded') and fold it into both isSegmentRecorded and recordedClauseIndicesForTools, the single recorded view every boundary guard reads. If the user can listen to the take, the segment under it is now locked, and it re-opens the moment the take is saved (latch cleared, optimistic set covers it) or cleared. Co-Authored-By: Claude Opus 4.8 (1M context) --- .../PassageDetailCarefulSpeech.test.tsx | 26 ++++++++++++++ .../PassageDetailGuidedPhraseRecord.tsx | 36 +++++++++++++------ 2 files changed, 52 insertions(+), 10 deletions(-) diff --git a/src/renderer/src/components/PassageDetail/PassageDetailCarefulSpeech.test.tsx b/src/renderer/src/components/PassageDetail/PassageDetailCarefulSpeech.test.tsx index 341803dd8..7e5eadcd9 100644 --- a/src/renderer/src/components/PassageDetail/PassageDetailCarefulSpeech.test.tsx +++ b/src/renderer/src/components/PassageDetail/PassageDetailCarefulSpeech.test.tsx @@ -706,6 +706,32 @@ describe('PassageDetailCarefulSpeech — rejected save (TT-7583)', () => { expect(controlsProps?.allowRecord).toBe(false); }); + it('locks the failed take’s clause across every boundary guard (TT-7437)', async () => { + await recordAndRejectSave(); + + // The upload failed but the take is still playable and latched to clause 2, + // so every boundary-editing guard must treat that clause as recorded — if + // the user can listen to the take, they must not reshape the segment under + // it, or a Retry would file the take against the altered boundaries. + const isRecorded = playerProps?.isSegmentRecorded as (i: number) => boolean; + expect(isRecorded(2)).toBe(true); // drag / +- guard + expect(controlsProps?.canCombineWithNext).toBe(false); // Split/Combine agree + }); + + it('re-opens the clause for editing once the failed take is cleared (TT-7437)', async () => { + mockRecordingRow = undefined; // nothing persisted; clearing drops the take + await recordAndRejectSave(); + + await act(async () => { + (controlsProps?.onClearRecording as () => void)(); + }); + + // Take gone: the same guards let clause 2 be edited again, in step. + const isRecorded = playerProps?.isSegmentRecorded as (i: number) => boolean; + expect(isRecorded(2)).toBe(false); + expect(controlsProps?.canCombineWithNext).toBe(true); + }); + it('clearing a failed take resets the recorder even with no mediafile', async () => { mockRecordingRow = undefined; // nothing was stored, so nothing to remove await recordAndRejectSave(); diff --git a/src/renderer/src/components/PassageDetail/PassageDetailGuidedPhraseRecord.tsx b/src/renderer/src/components/PassageDetail/PassageDetailGuidedPhraseRecord.tsx index 90aed3754..84f0051de 100644 --- a/src/renderer/src/components/PassageDetail/PassageDetailGuidedPhraseRecord.tsx +++ b/src/renderer/src/components/PassageDetail/PassageDetailGuidedPhraseRecord.tsx @@ -483,28 +483,44 @@ export function PassageDetailGuidedPhraseRecord({ ] ); + // A take that finished recording but has not saved yet — its upload failed + // and it is held for Retry — is still playable and stays latched to the + // clause it was recorded on. Treat that clause as recorded so its boundaries + // lock like a saved take's: if the user can listen to the take, they must not + // reshape the segment under it, or a Retry would file it against the altered + // boundaries (TT-7437). A successful save clears the latch (and the optimistic + // set then covers the clause), so this only fires for an unsaved take. + const pendingTakeIndex = + phase === 'recorded' ? recordingTarget?.index : undefined; + // A segment is treated as recorded (boundary locked, TT-7666) when it has a - // saved take, or a newly saved take that rowData has not shown yet. + // saved take, a newly saved take that rowData has not shown yet, or a + // recorded-but-unsaved take latched to it (see pendingTakeIndex above). // This only applies during the recording pass. const isSegmentRecorded = useCallback( (index: number) => recordingPassStarted && (completedIndices.has(index) || - optimisticCompletedRef.current.has(index)), + optimisticCompletedRef.current.has(index) || + index === pendingTakeIndex), // optimisticVersion makes optimistic-set changes reactive so every guard // that reads this predicate (drag, +/-, Split/Combine) recomputes together. // eslint-disable-next-line react-hooks/exhaustive-deps - [recordingPassStarted, completedIndices, optimisticVersion] + [recordingPassStarted, completedIndices, optimisticVersion, pendingTakeIndex] ); - /** completedIndices plus the optimistic just-saved set — the single recorded - * view every boundary-editing guard uses, so they agree in the window before - * rowData catches up (TT-7666). */ - const recordedClauseIndicesForTools = useMemo( - () => new Set([...completedIndices, ...optimisticCompletedRef.current]), + /** completedIndices plus the optimistic just-saved set and any latched + * unsaved take — the single recorded view every boundary-editing guard uses, + * so they agree in the window before rowData catches up (TT-7666, TT-7437). */ + const recordedClauseIndicesForTools = useMemo(() => { + const recorded = new Set([ + ...completedIndices, + ...optimisticCompletedRef.current, + ]); + if (pendingTakeIndex !== undefined) recorded.add(pendingTakeIndex); + return recorded; // eslint-disable-next-line react-hooks/exhaustive-deps - [completedIndices, optimisticVersion] - ); + }, [completedIndices, optimisticVersion, pendingTakeIndex]); const allClausesComplete = useMemo( () => From 9b64838727a9402cc5da2a4301db1d49db77780d Mon Sep 17 00:00:00 2001 From: Noel Chou Date: Fri, 4 Sep 2026 19:19:43 -0400 Subject: [PATCH 21/27] shorten a comment --- .../PassageDetail/PassageDetailGuidedPhraseRecord.tsx | 3 +-- 1 file changed, 1 insertion(+), 2 deletions(-) diff --git a/src/renderer/src/components/PassageDetail/PassageDetailGuidedPhraseRecord.tsx b/src/renderer/src/components/PassageDetail/PassageDetailGuidedPhraseRecord.tsx index 84f0051de..57f7a0064 100644 --- a/src/renderer/src/components/PassageDetail/PassageDetailGuidedPhraseRecord.tsx +++ b/src/renderer/src/components/PassageDetail/PassageDetailGuidedPhraseRecord.tsx @@ -486,8 +486,7 @@ export function PassageDetailGuidedPhraseRecord({ // A take that finished recording but has not saved yet — its upload failed // and it is held for Retry — is still playable and stays latched to the // clause it was recorded on. Treat that clause as recorded so its boundaries - // lock like a saved take's: if the user can listen to the take, they must not - // reshape the segment under it, or a Retry would file it against the altered + // lock like a saved take's, otherwise a Retry would file it against the altered // boundaries (TT-7437). A successful save clears the latch (and the optimistic // set then covers the clause), so this only fires for an unsaved take. const pendingTakeIndex = From 606e52566fdd28e954ddb59c2a61f3af4e6f2984 Mon Sep 17 00:00:00 2001 From: Noel Chou Date: Tue, 8 Sep 2026 09:19:03 -0400 Subject: [PATCH 22/27] TT-7666 drop the recorded-boundary backstop, rely on disabled handles MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit The region-update/region-updated handlers early-returned on isRecordedBoundary as a "backstop" for a drag already in flight, but that only suppressed the callbacks — it never reverted the geometry the regions plugin had already applied, so it could not actually enforce immutability. Because the segmentation string is serialized from the live region geometry, a suppressed-but-moved boundary would be laundered into the model by the next re-serialization (a +/-, split, or unrelated drag) before the next reload. The real protection is the drag guard: applyBoundaryEditability disables the resize handles (resizeStart/resizeEnd) on recorded boundaries and their shared neighbor edges, so the drag cannot start in the first place. The only case the backstop added over that was a segment being reclassified recorded mid-drag, which in single-user use is prevented by the recording lock (segments are locked for the whole save) — leaving only an out-of-band sync reclassifying a segment the user was legitimately editing as unrecorded. Not worth a guard that cannot enforce the invariant anyway. Remove the two isRecordedBoundary early-returns and the now-dead helper, and the three tests that drove the backstop by emitting region-update directly (bypassing the handle guard). The handle-disabling tests that cover the actual protection remain. Co-Authored-By: Claude Opus 4.8 (1M context) --- .../src/crud/useWavesurferRegions.test.tsx | 40 ------------------- .../src/crud/useWavesurferRegions.tsx | 20 ---------- 2 files changed, 60 deletions(-) diff --git a/src/renderer/src/crud/useWavesurferRegions.test.tsx b/src/renderer/src/crud/useWavesurferRegions.test.tsx index e7db0e7f8..c5a68cbf8 100644 --- a/src/renderer/src/crud/useWavesurferRegions.test.tsx +++ b/src/renderer/src/crud/useWavesurferRegions.test.tsx @@ -393,46 +393,6 @@ describe('useWaveSurferRegions — boundary drag on a recorded segment (TT-7666) // Recorded boundaries are frozen (TT-7666). Since a boundary is shared, // drag is blocked when either neighboring segment is recorded. - it('refuses to move a recorded segment via its own boundary', () => { - // Segment 1 is recorded. Dragging its end would resize it. - const { plugin, segs, onCurrentRegion } = renderRegions({ - lockSegmentSelection: false, - recordedIndices: [1], - }); - - dragBoundary(plugin, segs[1], 'end', 22); - - // Neighbor stays unchanged and no update is emitted. - expect(segs[2].start).toBe(20); - expect(onCurrentRegion).not.toHaveBeenCalled(); - }); - - it('refuses to move a recorded neighbour via the shared boundary', () => { - // Dragging this edge would also move recorded segment 2, so block it. - const { plugin, segs, onCurrentRegion } = renderRegions({ - lockSegmentSelection: false, - recordedIndices: [2], - }); - - dragBoundary(plugin, segs[1], 'end', 22); - - expect(segs[2].start).toBe(20); - expect(onCurrentRegion).not.toHaveBeenCalled(); - }); - - it('refuses a start-side drag that would reshape a recorded neighbour', () => { - // Segment 0 is recorded; segment 1's start is its shared boundary. - const { plugin, segs, onCurrentRegion } = renderRegions({ - lockSegmentSelection: false, - recordedIndices: [0], - }); - - dragBoundary(plugin, segs[1], 'start', 8); - - expect(segs[0].end).toBe(10); - expect(onCurrentRegion).not.toHaveBeenCalled(); - }); - it('still allows dragging a boundary between two unrecorded segments', () => { // This boundary is between unrecorded segments, so it stays draggable. const { plugin, segs, onCurrentRegion } = renderRegions({ diff --git a/src/renderer/src/crud/useWavesurferRegions.tsx b/src/renderer/src/crud/useWavesurferRegions.tsx index c29a6d6e4..29f42a3b6 100644 --- a/src/renderer/src/crud/useWavesurferRegions.tsx +++ b/src/renderer/src/crud/useWavesurferRegions.tsx @@ -299,21 +299,6 @@ export function useWaveSurferRegions( }); }; - /** True when the dragged boundary touches a recorded segment (TT-7666). */ - const isRecordedBoundary = (r: Region, side?: UpdateSide) => { - const recorded = isSegmentRecordedRef.current; - if (!recorded) return false; - const idx = regionIndexInSorted(r); - if (idx < 0) return false; - if (recorded(idx)) return true; - // 'start' shares previous, 'end' shares next; unknown side checks both. - if (side !== 'end' && idx > 0 && recorded(idx - 1)) return true; - if (side !== 'start' && idx < numRegions() - 1 && recorded(idx + 1)) { - return true; - } - return false; - }; - const Regions = () => regionsRef.current; const regions = () => Regions() @@ -641,9 +626,6 @@ export function useWaveSurferRegions( 'region-update', function (r: Region, side?: UpdateSide) { if (lockSegmentSelectionRef.current) return; - // Backstop: if a drag was already in flight, still block recorded - // boundaries here (TT-7666). - if (isRecordedBoundary(r, side)) return; resizingRef.current = r.resize; // Live-clamp the boundary as the user drags so regions never visually // overlap: the dragged boundary stops at the neighbor's edge and the @@ -659,8 +641,6 @@ export function useWaveSurferRegions( // region-updated can change selection and boundaries, so block it // while recording lock is active (TT-7437). if (lockSegmentSelectionRef.current) return; - // Backstop for in-flight drags on recorded boundaries (TT-7666). - if (isRecordedBoundary(r, side)) return; if (singleRegionRef.current) { if (!loadingRef.current) { waitForIt( From 95e84a32bc60b79ab99f276d6930e4c95e522d10 Mon Sep 17 00:00:00 2001 From: Noel Chou Date: Wed, 9 Sep 2026 09:19:01 -0400 Subject: [PATCH 23/27] TT-7666 gate the Record button on the shared recorded view allowRecord decided a clause was still recordable from completedIndices alone, while every boundary guard reads recordedClauseIndicesForTools (completed + optimistic + pending). In the window after a take saves but before rowData catches up, that let the just-recorded clause be re-recorded even though the clause was already marked recorded everywhere else. Point allowRecord at the same recordedClauseIndicesForTools so "is this clause already recorded?" is answered identically for Record, boundary drag, +/-, and Split/Combine. The pending part cannot over-block: it is only set while phase is 'recorded', where allowRecord is already false on the phase gate, so the only added input is the optimistic set - which a Clear removes, the intended way to re-record. Co-Authored-By: Claude Opus 4.8 (1M context) --- .../PassageDetail/PassageDetailGuidedPhraseRecord.tsx | 5 ++++- 1 file changed, 4 insertions(+), 1 deletion(-) diff --git a/src/renderer/src/components/PassageDetail/PassageDetailGuidedPhraseRecord.tsx b/src/renderer/src/components/PassageDetail/PassageDetailGuidedPhraseRecord.tsx index 57f7a0064..3a145397e 100644 --- a/src/renderer/src/components/PassageDetail/PassageDetailGuidedPhraseRecord.tsx +++ b/src/renderer/src/components/PassageDetail/PassageDetailGuidedPhraseRecord.tsx @@ -1809,7 +1809,10 @@ export function PassageDetailGuidedPhraseRecord({ recordingPassStarted && currentClausePlayed && (phase === 'recordReady' || phase === 'recording') && - !completedIndices.has(currentIndex); + // Same recorded view the boundary guards use (completed + optimistic + + // pending), so a just-saved clause can't be re-recorded before rowData + // catches up (TT-7666). + !recordedClauseIndicesForTools.has(currentIndex); /** * Disable only the Record button until expected clause playback time expires. From 524f282f6dea8768b7ebae3f110be9f4a91444e2 Mon Sep 17 00:00:00 2001 From: Noel Chou Date: Wed, 9 Sep 2026 09:35:37 -0400 Subject: [PATCH 24/27] TT-7666 track optimistic/pending takes by boundaries so they can't drift The optimistic-completion set and the pending-take marker stored raw clause indices. completedIndices, by contrast, re-derives its indices every recompute by matching saved mediafiles to the live regions (getCompletedClauseIndices), so it follows a re-segmentation; the index-based sets could not. Save a take, then split or combine an earlier clause, and every clause from it shifts by one: completedIndices remaps to the take's new index, but the optimistic index stays put - now pointing at a different clause. That both released the guards on the clause that actually holds the take and locked an unrelated clause (blocking Record there, via the shared recorded view), until a step reset. Give the optimistic/pending tracking the same anchor completedIndices uses. Store each optimistic take by the boundaries it was cut against (the take's region, already latched at record start) instead of an index, and add clauseIndexForRegion to re-match those boundaries to the current clause layout. optimisticCompletedIndices and pendingTakeIndex are now derived by matching, so they track splits/combines of earlier clauses exactly as completedIndices does, and the reconciliation effect drops an optimistic take once rowData confirms the clause it currently maps to. add/remove/clear now take a region; the upload callbacks pass the take's latched region (falling back to the live current region via a ref for the rare unlatched case). carefulSpeechBoundary.test.ts covers clauseIndexForRegion, including a take following its clause to a new index after an earlier split. Co-Authored-By: Claude Opus 4.8 (1M context) --- .../PassageDetailGuidedPhraseRecord.tsx | 117 ++++++++++++------ .../carefulSpeechBoundary.test.ts | 30 +++++ .../carefulSpeech/carefulSpeechBoundary.ts | 20 +++ 3 files changed, 129 insertions(+), 38 deletions(-) diff --git a/src/renderer/src/components/PassageDetail/PassageDetailGuidedPhraseRecord.tsx b/src/renderer/src/components/PassageDetail/PassageDetailGuidedPhraseRecord.tsx index 3a145397e..0ef4a268c 100644 --- a/src/renderer/src/components/PassageDetail/PassageDetailGuidedPhraseRecord.tsx +++ b/src/renderer/src/components/PassageDetail/PassageDetailGuidedPhraseRecord.tsx @@ -39,6 +39,7 @@ import { useGuidedPhraseSegments } from './carefulSpeech/useGuidedPhraseSegments import { resolveSegmentSpeaker } from './carefulSpeech/resolveSegmentSpeaker'; import { CLAUSE_BOUNDARY_THRESHOLD_SEC, + clauseIndexForRegion, hasPhraseRegions, preservesRecordedBoundaries, regionBoundariesEqual, @@ -245,32 +246,56 @@ export function PassageDetailGuidedPhraseRecord({ // Armed after auto-play park to ignore one spurious +1 clause jump // (overshoot/region-in) while still allowing real navigation (TT-7360). const pendingOvershootSwallowRef = useRef(false); - /** Session-local saved indices before rowData catches up (TT-7552, TT-7666). */ - const optimisticCompletedRef = useRef>(new Set()); + // Takes saved this session but not yet shown by rowData (TT-7552, TT-7666). + // Held by the boundaries each take was cut against, not a raw index, so the + // clause is re-matched to the live layout every recompute and cannot drift + // when an earlier clause is split or combined (see clauseIndexForRegion). + const optimisticTakeRegionsRef = useRef([]); const [optimisticVersion, setOptimisticVersion] = useState(0); const bumpOptimistic = useCallback( () => setOptimisticVersion((v) => v + 1), [] ); + const sameRegion = useCallback( + (a: IRegion, b: IRegion) => + Math.abs(a.start - b.start) < 0.05 && Math.abs(a.end - b.end) < 0.05, + [] + ); const addOptimistic = useCallback( - (index: number) => { - optimisticCompletedRef.current.add(index); + (region: IRegion | undefined) => { + if (!region) return; + if (optimisticTakeRegionsRef.current.some((r) => sameRegion(r, region))) { + return; + } + optimisticTakeRegionsRef.current = [ + ...optimisticTakeRegionsRef.current, + { start: region.start, end: region.end }, + ]; bumpOptimistic(); }, - [bumpOptimistic] + [bumpOptimistic, sameRegion] ); const removeOptimistic = useCallback( - (index: number) => { - if (optimisticCompletedRef.current.delete(index)) bumpOptimistic(); + (region: IRegion | undefined) => { + if (!region) return; + const kept = optimisticTakeRegionsRef.current.filter( + (r) => !sameRegion(r, region) + ); + if (kept.length !== optimisticTakeRegionsRef.current.length) { + optimisticTakeRegionsRef.current = kept; + bumpOptimistic(); + } }, - [bumpOptimistic] + [bumpOptimistic, sameRegion] ); const clearOptimistic = useCallback(() => { - if (optimisticCompletedRef.current.size === 0) return; - optimisticCompletedRef.current.clear(); + if (optimisticTakeRegionsRef.current.length === 0) return; + optimisticTakeRegionsRef.current = []; bumpOptimistic(); }, [bumpOptimistic]); const currentIndexRef = useRef(0); + // Live current-clause region, for the upload callbacks (fire outside render). + const currentRegionRef = useRef(undefined); /** Latched clause/region for the active take until save or discard (TT-7437). */ const [recordingTarget, setRecordingTarget] = useState< { index: number; region: IRegion } | undefined @@ -483,14 +508,31 @@ export function PassageDetailGuidedPhraseRecord({ ] ); + // Current clause indices of the optimistically-saved takes, re-matched to the + // live layout by boundaries so they follow when an earlier clause is split or + // combined, exactly as completedIndices does (TT-7666). + const optimisticCompletedIndices = useMemo(() => { + const indices = new Set(); + for (const region of optimisticTakeRegionsRef.current) { + const index = clauseIndexForRegion(region, clauseRegions); + if (index >= 0) indices.add(index); + } + return indices; + // eslint-disable-next-line react-hooks/exhaustive-deps + }, [clauseRegions, optimisticVersion]); + // A take that finished recording but has not saved yet — its upload failed // and it is held for Retry — is still playable and stays latched to the // clause it was recorded on. Treat that clause as recorded so its boundaries // lock like a saved take's, otherwise a Retry would file it against the altered // boundaries (TT-7437). A successful save clears the latch (and the optimistic - // set then covers the clause), so this only fires for an unsaved take. - const pendingTakeIndex = - phase === 'recorded' ? recordingTarget?.index : undefined; + // set then covers the clause), so this only fires for an unsaved take. Matched + // by boundaries, like the optimistic set, so it tracks the live layout. + const pendingTakeIndex = useMemo(() => { + if (phase !== 'recorded' || !recordingTarget) return undefined; + const index = clauseIndexForRegion(recordingTarget.region, clauseRegions); + return index >= 0 ? index : undefined; + }, [phase, recordingTarget, clauseRegions]); // A segment is treated as recorded (boundary locked, TT-7666) when it has a // saved take, a newly saved take that rowData has not shown yet, or a @@ -500,12 +542,14 @@ export function PassageDetailGuidedPhraseRecord({ (index: number) => recordingPassStarted && (completedIndices.has(index) || - optimisticCompletedRef.current.has(index) || + optimisticCompletedIndices.has(index) || index === pendingTakeIndex), - // optimisticVersion makes optimistic-set changes reactive so every guard - // that reads this predicate (drag, +/-, Split/Combine) recomputes together. - // eslint-disable-next-line react-hooks/exhaustive-deps - [recordingPassStarted, completedIndices, optimisticVersion, pendingTakeIndex] + [ + recordingPassStarted, + completedIndices, + optimisticCompletedIndices, + pendingTakeIndex, + ] ); /** completedIndices plus the optimistic just-saved set and any latched @@ -514,12 +558,11 @@ export function PassageDetailGuidedPhraseRecord({ const recordedClauseIndicesForTools = useMemo(() => { const recorded = new Set([ ...completedIndices, - ...optimisticCompletedRef.current, + ...optimisticCompletedIndices, ]); if (pendingTakeIndex !== undefined) recorded.add(pendingTakeIndex); return recorded; - // eslint-disable-next-line react-hooks/exhaustive-deps - }, [completedIndices, optimisticVersion, pendingTakeIndex]); + }, [completedIndices, optimisticCompletedIndices, pendingTakeIndex]); const allClausesComplete = useMemo( () => @@ -629,29 +672,28 @@ export function PassageDetailGuidedPhraseRecord({ currentIndex, isCompleted: (i) => recordingPassStarted - ? completedIndices.has(i) || optimisticCompletedRef.current.has(i) + ? completedIndices.has(i) || optimisticCompletedIndices.has(i) : heardSet.has(i), }; currentIndexRef.current = currentIndex; + currentRegionRef.current = currentRegion; const applyColors = useCallback(() => { playerControlsRef.current?.applyRegionColors?.(); }, []); - // Drop optimistic flags once rowData confirms those clauses. + // Drop an optimistic take once rowData confirms the clause it now maps to. useEffect(() => { - let changed = false; - for (const i of [...optimisticCompletedRef.current]) { - if (completedIndices.has(i)) { - optimisticCompletedRef.current.delete(i); - changed = true; - } - } - if (changed) { + const kept = optimisticTakeRegionsRef.current.filter((region) => { + const index = clauseIndexForRegion(region, clauseRegions); + return !(index >= 0 && completedIndices.has(index)); + }); + if (kept.length !== optimisticTakeRegionsRef.current.length) { + optimisticTakeRegionsRef.current = kept; bumpOptimistic(); applyColors(); } - }, [completedIndices, applyColors, bumpOptimistic]); + }, [completedIndices, clauseRegions, applyColors, bumpOptimistic]); const bumpSuppressClauseAutoPlay = useCallback((count = 1) => { suppressClauseAutoPlayRef.current += count; @@ -1741,14 +1783,13 @@ export function PassageDetailGuidedPhraseRecord({ // Mark optimistic completion immediately after real upload (TT-7552), // and always apply it to the latched recording-start clause (TT-7437). // No mediaId means upload failed; do not show optimistic success (TT-7583). - const takenIndex = - recordingTargetRef.current?.index ?? currentIndexRef.current; + const takeRegion = recordingTargetRef.current?.region ?? currentRegionRef.current; if (mediaId) { - addOptimistic(takenIndex); + addOptimistic(takeRegion); // Stored: the take is no longer pending, so release the clause. latchRecordingTarget(undefined); } else { - removeOptimistic(takenIndex); + removeOptimistic(takeRegion); // Keep the latch on failed upload so Retry files to the same clause // even if selection moved (TT-7583). } @@ -1787,7 +1828,7 @@ export function PassageDetailGuidedPhraseRecord({ } } removeOptimistic( - recordingTargetRef.current?.index ?? currentIndexRef.current + recordingTargetRef.current?.region ?? currentRegionRef.current ); // The take is gone, so the clause it was held against is released too. latchRecordingTarget(undefined); @@ -2026,7 +2067,7 @@ export function PassageDetailGuidedPhraseRecord({ // Also clear optimistic green on this failure path (TT-7583). // Keep the latch so Retry still files to the same clause. removeOptimistic( - recordingTargetRef.current?.index ?? currentIndexRef.current + recordingTargetRef.current?.region ?? currentRegionRef.current ); applyColors(); }} diff --git a/src/renderer/src/components/PassageDetail/carefulSpeech/carefulSpeechBoundary.test.ts b/src/renderer/src/components/PassageDetail/carefulSpeech/carefulSpeechBoundary.test.ts index 1540f4e89..0bcfd1063 100644 --- a/src/renderer/src/components/PassageDetail/carefulSpeech/carefulSpeechBoundary.test.ts +++ b/src/renderer/src/components/PassageDetail/carefulSpeech/carefulSpeechBoundary.test.ts @@ -1,5 +1,6 @@ import { describe, expect, it } from '@jest/globals'; import { + clauseIndexForRegion, preservesRecordedBoundaries, regionBoundariesEqual, } from './carefulSpeechBoundary'; @@ -55,3 +56,32 @@ describe('preservesRecordedBoundaries', () => { ).toBe(false); }); }); + +describe('clauseIndexForRegion', () => { + const regions = [ + { start: 0, end: 10 }, + { start: 10, end: 20 }, + { start: 20, end: 30 }, + ]; + + it('finds the clause whose boundaries match', () => { + expect(clauseIndexForRegion({ start: 10, end: 20 }, regions)).toBe(1); + }); + + it('tracks a take to its new index after an earlier clause splits', () => { + // A take was recorded on { 10, 20 } (index 1). Splitting clause 0 shifts it + // to index 2, but its boundaries are unchanged — the match follows. + const afterSplit = [ + { start: 0, end: 5 }, + { start: 5, end: 10 }, + { start: 10, end: 20 }, + { start: 20, end: 30 }, + ]; + expect(clauseIndexForRegion({ start: 10, end: 20 }, afterSplit)).toBe(2); + }); + + it('matches within tolerance and returns -1 when no clause lines up', () => { + expect(clauseIndexForRegion({ start: 10.02, end: 19.98 }, regions)).toBe(1); + expect(clauseIndexForRegion({ start: 12, end: 18 }, regions)).toBe(-1); + }); +}); diff --git a/src/renderer/src/components/PassageDetail/carefulSpeech/carefulSpeechBoundary.ts b/src/renderer/src/components/PassageDetail/carefulSpeech/carefulSpeechBoundary.ts index 1a3799d02..32cc542be 100644 --- a/src/renderer/src/components/PassageDetail/carefulSpeech/carefulSpeechBoundary.ts +++ b/src/renderer/src/components/PassageDetail/carefulSpeech/carefulSpeechBoundary.ts @@ -75,5 +75,25 @@ export function preservesRecordedBoundaries( return true; } +/** + * Index of the clause whose boundaries match `region`, or -1. Lets callers + * remember a recorded take by the boundaries it was cut against and re-derive + * its current clause index after the clauses are re-segmented — the same way + * getCompletedClauseIndices re-matches saved mediafiles to the live regions, + * so index tracking cannot drift when an earlier clause is split or combined + * (TT-7666). + */ +export function clauseIndexForRegion( + region: IRegion, + clauseRegions: IRegion[], + tolerance = REGION_EQ_TOLERANCE +): number { + return clauseRegions.findIndex( + (c) => + Math.abs(c.start - region.start) < tolerance && + Math.abs(c.end - region.end) < tolerance + ); +} + /** @deprecated Prefer hasPhraseRegions — kept for BOLD clause naming at call sites. */ export const hasClauseRegions = hasPhraseRegions; From 9f8bbef150c55c1a6c0a27243ecec1f5fbf1a37b Mon Sep 17 00:00:00 2001 From: Noel Chou Date: Wed, 9 Sep 2026 09:38:19 -0400 Subject: [PATCH 25/27] TT-7666 share one region-boundary match rule across the clause matchers The "same start/end within 0.05" test was copied in five places, each with its own literal tolerance: regionBoundariesEqual, preservesRecordedBoundaries, the new clauseIndexForRegion, regionMatchesClause (the completedIndices matcher), and a local sameRegion in the record component. The optimistic/pending remap and the completedIndices remap are supposed to agree on what counts as the same clause; with the rule duplicated they only agreed by coincidence. Extract regionsMatch(a, b, tolerance?) and one exported REGION_EQ_TOLERANCE, and route all of them through it: regionBoundariesEqual, preservesRecordedBoundaries, clauseIndexForRegion, and getRecordingForClause's regionMatchesClause now share the identical equality, and the component drops sameRegion for it. Changing the tolerance now moves both remap paths together instead of leaving them to drift. Note: regionBoundariesEqual previously treated a difference of exactly the tolerance as equal (> tolerance) whereas regionsMatch uses < tolerance, matching the other four call sites; the exact-boundary case is not reachable with real region times and this makes the rule uniform. Co-Authored-By: Claude Opus 4.8 (1M context) --- .../PassageDetailGuidedPhraseRecord.tsx | 14 +++---- .../carefulSpeech/carefulSpeechBoundary.ts | 38 +++++++++---------- .../carefulSpeech/carefulSpeechCompletion.ts | 8 +--- 3 files changed, 25 insertions(+), 35 deletions(-) diff --git a/src/renderer/src/components/PassageDetail/PassageDetailGuidedPhraseRecord.tsx b/src/renderer/src/components/PassageDetail/PassageDetailGuidedPhraseRecord.tsx index 0ef4a268c..dcc701353 100644 --- a/src/renderer/src/components/PassageDetail/PassageDetailGuidedPhraseRecord.tsx +++ b/src/renderer/src/components/PassageDetail/PassageDetailGuidedPhraseRecord.tsx @@ -44,6 +44,7 @@ import { preservesRecordedBoundaries, regionBoundariesEqual, regionsJsonFromList, + regionsMatch, } from './carefulSpeech/carefulSpeechBoundary'; import { firstIncompleteClauseIndex, @@ -256,15 +257,10 @@ export function PassageDetailGuidedPhraseRecord({ () => setOptimisticVersion((v) => v + 1), [] ); - const sameRegion = useCallback( - (a: IRegion, b: IRegion) => - Math.abs(a.start - b.start) < 0.05 && Math.abs(a.end - b.end) < 0.05, - [] - ); const addOptimistic = useCallback( (region: IRegion | undefined) => { if (!region) return; - if (optimisticTakeRegionsRef.current.some((r) => sameRegion(r, region))) { + if (optimisticTakeRegionsRef.current.some((r) => regionsMatch(r, region))) { return; } optimisticTakeRegionsRef.current = [ @@ -273,20 +269,20 @@ export function PassageDetailGuidedPhraseRecord({ ]; bumpOptimistic(); }, - [bumpOptimistic, sameRegion] + [bumpOptimistic] ); const removeOptimistic = useCallback( (region: IRegion | undefined) => { if (!region) return; const kept = optimisticTakeRegionsRef.current.filter( - (r) => !sameRegion(r, region) + (r) => !regionsMatch(r, region) ); if (kept.length !== optimisticTakeRegionsRef.current.length) { optimisticTakeRegionsRef.current = kept; bumpOptimistic(); } }, - [bumpOptimistic, sameRegion] + [bumpOptimistic] ); const clearOptimistic = useCallback(() => { if (optimisticTakeRegionsRef.current.length === 0) return; diff --git a/src/renderer/src/components/PassageDetail/carefulSpeech/carefulSpeechBoundary.ts b/src/renderer/src/components/PassageDetail/carefulSpeech/carefulSpeechBoundary.ts index 32cc542be..e250a885a 100644 --- a/src/renderer/src/components/PassageDetail/carefulSpeech/carefulSpeechBoundary.ts +++ b/src/renderer/src/components/PassageDetail/carefulSpeech/carefulSpeechBoundary.ts @@ -21,7 +21,21 @@ export function hasPhraseRegions(segmentsJson: string): boolean { } } -const REGION_EQ_TOLERANCE = 0.05; +export const REGION_EQ_TOLERANCE = 0.05; + +/** True when two regions share the same start/end within tolerance (labels + * ignored). The single boundary-equality rule every clause matcher builds on, + * so they cannot disagree if the tolerance changes (TT-7666). */ +export function regionsMatch( + a: IRegion, + b: IRegion, + tolerance = REGION_EQ_TOLERANCE +): boolean { + return ( + Math.abs(a.start - b.start) < tolerance && + Math.abs(a.end - b.end) < tolerance + ); +} /** True when both maps have the same start/end boundaries (labels ignored). */ export function regionBoundariesEqual( @@ -32,15 +46,7 @@ export function regionBoundariesEqual( const a = parseRegionList(aJson); const b = parseRegionList(bJson); if (a.length !== b.length) return false; - for (let i = 0; i < a.length; i++) { - if ( - Math.abs(a[i].start - b[i].start) > tolerance || - Math.abs(a[i].end - b[i].end) > tolerance - ) { - return false; - } - } - return true; + return a.every((region, i) => regionsMatch(region, b[i], tolerance)); } function parseRegionList(segmentsJson: string): IRegion[] { @@ -65,11 +71,7 @@ export function preservesRecordedBoundaries( for (const i of completed) { const r = oldRegions[i]; if (!r) continue; - const stillExists = newRegions.some( - (n) => - Math.abs(n.start - r.start) < tolerance && - Math.abs(n.end - r.end) < tolerance - ); + const stillExists = newRegions.some((n) => regionsMatch(n, r, tolerance)); if (!stillExists) return false; } return true; @@ -88,11 +90,7 @@ export function clauseIndexForRegion( clauseRegions: IRegion[], tolerance = REGION_EQ_TOLERANCE ): number { - return clauseRegions.findIndex( - (c) => - Math.abs(c.start - region.start) < tolerance && - Math.abs(c.end - region.end) < tolerance - ); + return clauseRegions.findIndex((c) => regionsMatch(c, region, tolerance)); } /** @deprecated Prefer hasPhraseRegions — kept for BOLD clause naming at call sites. */ diff --git a/src/renderer/src/components/PassageDetail/carefulSpeech/carefulSpeechCompletion.ts b/src/renderer/src/components/PassageDetail/carefulSpeech/carefulSpeechCompletion.ts index 576c82bfe..d9a6d9af0 100644 --- a/src/renderer/src/components/PassageDetail/carefulSpeech/carefulSpeechCompletion.ts +++ b/src/renderer/src/components/PassageDetail/carefulSpeech/carefulSpeechCompletion.ts @@ -1,13 +1,12 @@ import { IRegion } from '../../../crud/useWavesurferRegions'; import { IRow } from '../../../context/PassageDetailContext'; import { prettySegment } from '../../../utils/prettySegment'; +import { regionsMatch } from './carefulSpeechBoundary'; import { matchesGuidedOutputRow, pickLatestGuidedOutputRow, } from './matchesGuidedOutputRow'; -const REGION_TOLERANCE = 0.05; - function isEmptySourceSegments(seg: string | undefined): boolean { if (!seg) return true; const trimmed = seg.trim(); @@ -47,10 +46,7 @@ function regionMatchesClause( } const stored = parseStoredRegion(storedSeg); if (stored) { - return ( - Math.abs(stored.start - clauseRegion.start) < REGION_TOLERANCE && - Math.abs(stored.end - clauseRegion.end) < REGION_TOLERANCE - ); + return regionsMatch(stored, clauseRegion); } return prettySegment(storedSeg).trim() === prettySegment(clauseRegion).trim(); } From 51464c9c08cffacf5fb17ee0988585fe8b53f49c Mon Sep 17 00:00:00 2001 From: Noel Chou Date: Wed, 9 Sep 2026 09:47:38 -0400 Subject: [PATCH 26/27] TT-7666 use the shared recorded view in the handleSegment reload backstop handleSegment's defense-in-depth reload checked preservesRecordedBoundaries against completedIndices alone, unlike every other guard, which reads recordedClauseIndicesForTools (completed + optimistic + pending). In the window after a take saves but before rowData catches up, a segment change that reshaped the just-saved clause would slip past this last check. Point it at recordedClauseIndicesForTools so it agrees with the drag, +/-, and Split/Combine guards. With optimistic/pending now matched by boundaries, this view no longer drifts either. Co-Authored-By: Claude Opus 4.8 (1M context) --- .../PassageDetail/PassageDetailGuidedPhraseRecord.tsx | 11 +++++++++-- 1 file changed, 9 insertions(+), 2 deletions(-) diff --git a/src/renderer/src/components/PassageDetail/PassageDetailGuidedPhraseRecord.tsx b/src/renderer/src/components/PassageDetail/PassageDetailGuidedPhraseRecord.tsx index dcc701353..cb0ea9a05 100644 --- a/src/renderer/src/components/PassageDetail/PassageDetailGuidedPhraseRecord.tsx +++ b/src/renderer/src/components/PassageDetail/PassageDetailGuidedPhraseRecord.tsx @@ -1186,9 +1186,16 @@ export function PassageDetailGuidedPhraseRecord({ // Defense-in-depth only: if an update still changes recorded boundaries, // reload the previous regions. Main blocking now happens earlier in // useWavesurferRegions (drag, split/merge, and +/- controls; TT-7666). + // Use the same recorded view every other guard reads (completed + + // optimistic + pending), so a just-saved clause is protected here too + // before rowData catches up. if ( recordingPassStarted && - !preservesRecordedBoundaries(clauseRegions, regions, completedIndices) + !preservesRecordedBoundaries( + clauseRegions, + regions, + recordedClauseIndicesForTools + ) ) { playerControlsRef.current?.loadRegionsJson?.(clauseSegString); return; @@ -1208,7 +1215,7 @@ export function PassageDetailGuidedPhraseRecord({ savingRecording, recordingPassStarted, clauseRegions, - completedIndices, + recordedClauseIndicesForTools, clauseSegString, pushSegmentUndo, ] From 95970528de2e3f204fb617fe734792715b99c530 Mon Sep 17 00:00:00 2001 From: Noel Chou Date: Wed, 9 Sep 2026 10:10:58 -0400 Subject: [PATCH 27/27] TT-7666 freeze recorded clauses at entry, not just in the recording pass isSegmentRecorded was gated on recordingPassStarted on the assumption that the listen pass has no takes. But recordingPassStarted is a state flag flipped inside the runInitialPosition bootstrap effect, while completedIndices is derived synchronously from rowData. Entering an already-recorded passage leaves a window where completedIndices is populated but the flag has not flipped yet, so a stored take's clause reported unrecorded and its boundaries stayed editable. Drop the phase gate and key the predicate on the recorded sets alone (saved + optimistic + pending). completedIndices is authoritative whenever a take exists, and optimistic/pending only populate once recording has started, so removing the gate closes the entry window without over-freezing: unrecorded clauses are still not in any set. Co-Authored-By: Claude Opus 4.8 (1M context) --- .../PassageDetailGuidedPhraseRecord.tsx | 19 ++++++++----------- 1 file changed, 8 insertions(+), 11 deletions(-) diff --git a/src/renderer/src/components/PassageDetail/PassageDetailGuidedPhraseRecord.tsx b/src/renderer/src/components/PassageDetail/PassageDetailGuidedPhraseRecord.tsx index cb0ea9a05..383430d89 100644 --- a/src/renderer/src/components/PassageDetail/PassageDetailGuidedPhraseRecord.tsx +++ b/src/renderer/src/components/PassageDetail/PassageDetailGuidedPhraseRecord.tsx @@ -533,19 +533,16 @@ export function PassageDetailGuidedPhraseRecord({ // A segment is treated as recorded (boundary locked, TT-7666) when it has a // saved take, a newly saved take that rowData has not shown yet, or a // recorded-but-unsaved take latched to it (see pendingTakeIndex above). - // This only applies during the recording pass. + // Keyed on the recorded sets alone: a stored take must freeze its clause even + // at entry, before runInitialPosition flips recordingPassStarted — that flag + // lags completedIndices (derived synchronously from rowData), so gating on it + // left an already-recorded clause editable during the initial-load window. const isSegmentRecorded = useCallback( (index: number) => - recordingPassStarted && - (completedIndices.has(index) || - optimisticCompletedIndices.has(index) || - index === pendingTakeIndex), - [ - recordingPassStarted, - completedIndices, - optimisticCompletedIndices, - pendingTakeIndex, - ] + completedIndices.has(index) || + optimisticCompletedIndices.has(index) || + index === pendingTakeIndex, + [completedIndices, optimisticCompletedIndices, pendingTakeIndex] ); /** completedIndices plus the optimistic just-saved set and any latched