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. diff --git a/src/renderer/src/components/PassageDetail/PassageDetailCarefulSpeech.test.tsx b/src/renderer/src/components/PassageDetail/PassageDetailCarefulSpeech.test.tsx index 4e7127fc2..7e5eadcd9 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,82 @@ describe('PassageDetailCarefulSpeech — recording segment lock (TT-7437)', () = }); }); +describe('PassageDetailCarefulSpeech — take belongs to the clause it started on (TT-7437)', () => { + // 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(); + 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 () => { + // 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 () => { + (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 +553,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 +584,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); @@ -634,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(); @@ -679,3 +777,71 @@ 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); + }); +}); + +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 4f7c03d96..383430d89 100644 --- a/src/renderer/src/components/PassageDetail/PassageDetailGuidedPhraseRecord.tsx +++ b/src/renderer/src/components/PassageDetail/PassageDetailGuidedPhraseRecord.tsx @@ -39,10 +39,12 @@ import { useGuidedPhraseSegments } from './carefulSpeech/useGuidedPhraseSegments import { resolveSegmentSpeaker } from './carefulSpeech/resolveSegmentSpeaker'; import { CLAUSE_BOUNDARY_THRESHOLD_SEC, + clauseIndexForRegion, hasPhraseRegions, preservesRecordedBoundaries, regionBoundariesEqual, regionsJsonFromList, + regionsMatch, } from './carefulSpeech/carefulSpeechBoundary'; import { firstIncompleteClauseIndex, @@ -206,12 +208,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 +235,78 @@ 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). */ - 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 addOptimistic = useCallback( + (region: IRegion | undefined) => { + if (!region) return; + if (optimisticTakeRegionsRef.current.some((r) => regionsMatch(r, region))) { + return; + } + optimisticTakeRegionsRef.current = [ + ...optimisticTakeRegionsRef.current, + { start: region.start, end: region.end }, + ]; + bumpOptimistic(); + }, + [bumpOptimistic] + ); + const removeOptimistic = useCallback( + (region: IRegion | undefined) => { + if (!region) return; + const kept = optimisticTakeRegionsRef.current.filter( + (r) => !regionsMatch(r, region) + ); + if (kept.length !== optimisticTakeRegionsRef.current.length) { + optimisticTakeRegionsRef.current = kept; + bumpOptimistic(); + } + }, + [bumpOptimistic] + ); + const clearOptimistic = useCallback(() => { + 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 + >(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); @@ -458,6 +504,59 @@ 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. 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 + // recorded-but-unsaved take latched to it (see pendingTakeIndex above). + // 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) => + completedIndices.has(index) || + optimisticCompletedIndices.has(index) || + index === pendingTakeIndex, + [completedIndices, optimisticCompletedIndices, pendingTakeIndex] + ); + + /** 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, + ...optimisticCompletedIndices, + ]); + if (pendingTakeIndex !== undefined) recorded.add(pendingTakeIndex); + return recorded; + }, [completedIndices, optimisticCompletedIndices, pendingTakeIndex]); + const allClausesComplete = useMemo( () => clauseRegions.length > 0 && completedIndices.size >= clauseRegions.length, @@ -531,9 +630,14 @@ 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, + takeIndex, currentVersion, stepLanguageBcp47 ); @@ -551,7 +655,7 @@ export function PassageDetailGuidedPhraseRecord({ memory, artifactTypeId, offline, - currentIndex, + takeIndex, currentVersion, stepLanguageBcp47, config, @@ -561,26 +665,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; - } + 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(); } - if (changed) applyColors(); - }, [completedIndices, applyColors]); + }, [completedIndices, clauseRegions, applyColors, bumpOptimistic]); const bumpSuppressClauseAutoPlay = useCallback((count = 1) => { suppressClauseAutoPlayRef.current += count; @@ -683,8 +789,13 @@ export function PassageDetailGuidedPhraseRecord({ // Keyed on the index rather than the navigation handlers because every clause // move funnels through it. useEffect(() => { + // 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); + // eslint-disable-next-line react-hooks/exhaustive-deps }, [currentIndex]); useEffect(() => { @@ -719,17 +830,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() + @@ -799,21 +901,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) { @@ -838,32 +929,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) => @@ -940,7 +1015,10 @@ export function PassageDetailGuidedPhraseRecord({ saveRejectedRef.current = false; setSaveRejected(false); pendingOvershootSwallowRef.current = false; - optimisticCompletedRef.current.clear(); + // 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); + clearOptimistic(); setHeardIndices([]); setCurrentClausePlayed(false); setCombineUndo(null); @@ -1024,9 +1102,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); @@ -1104,9 +1180,19 @@ export function PassageDetailGuidedPhraseRecord({ if (recordingActiveRef.current || savingRecording) return; const regions = getSortedRegions(seg); if (regions.length === 0) return; + // 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; @@ -1126,7 +1212,7 @@ export function PassageDetailGuidedPhraseRecord({ savingRecording, recordingPassStarted, clauseRegions, - completedIndices, + recordedClauseIndicesForTools, clauseSegString, pushSegmentUndo, ] @@ -1164,11 +1250,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'); @@ -1185,23 +1268,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; }, []); @@ -1225,17 +1300,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); @@ -1283,20 +1349,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, @@ -1403,7 +1458,7 @@ export function PassageDetailGuidedPhraseRecord({ playerControlsRef.current?.loadRegionsJson?.(baseline); setRecordingPassStarted(false); recordingPassStartedRef.current = false; - optimisticCompletedRef.current.clear(); + clearOptimistic(); setShowRecorder(false); setHeardIndices([]); setCurrentClausePlayed(false); @@ -1438,6 +1493,7 @@ export function PassageDetailGuidedPhraseRecord({ setStepComplete, forceRefresh, applyColors, + clearOptimistic, ]); const handleClearSegments = useCallback(async () => { @@ -1484,7 +1540,7 @@ export function PassageDetailGuidedPhraseRecord({ phraseSegParams ); if ( - !canSplitClause(currentIndex, clauseRegions, completedIndices, splitPoint) + !canSplitClause(currentIndex, clauseRegions, recordedClauseIndicesForTools, splitPoint) ) { return; } @@ -1508,7 +1564,7 @@ export function PassageDetailGuidedPhraseRecord({ }, [ currentIndex, clauseRegions, - completedIndices, + recordedClauseIndicesForTools, clauseSegString, phraseSegParams, setClauseSegString, @@ -1522,7 +1578,7 @@ export function PassageDetailGuidedPhraseRecord({ const handleCombineWithNext = useCallback(async () => { if (savingRecordingRef.current) return; - if (!canCombineWithNext(currentIndex, clauseRegions, completedIndices)) { + if (!canCombineWithNext(currentIndex, clauseRegions, recordedClauseIndicesForTools)) { return; } const updated = mergeClauseWithNext(clauseRegions, currentIndex); @@ -1541,7 +1597,7 @@ export function PassageDetailGuidedPhraseRecord({ }, [ currentIndex, clauseRegions, - completedIndices, + recordedClauseIndicesForTools, clauseSegString, phraseSegParams, setClauseSegString, @@ -1709,12 +1765,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, @@ -1728,18 +1780,21 @@ 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 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 takeRegion = recordingTargetRef.current?.region ?? currentRegionRef.current; if (mediaId) { - optimisticCompletedRef.current.add(currentIndexRef.current); + addOptimistic(takeRegion); + // Stored: the take is no longer pending, so release the clause. + latchRecordingTarget(undefined); } else { - optimisticCompletedRef.current.delete(currentIndexRef.current); + removeOptimistic(takeRegion); + // 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); @@ -1747,17 +1802,21 @@ export function PassageDetailGuidedPhraseRecord({ setResetMedia(false); applyColors(); }, - [forceRefresh, applyColors] + [ + forceRefresh, + applyColors, + latchRecordingTarget, + addOptimistic, + removeOptimistic, + ] ); 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) => @@ -1768,7 +1827,11 @@ export function PassageDetailGuidedPhraseRecord({ await setStepComplete(currentstep, false); } } - optimisticCompletedRef.current.delete(currentIndexRef.current); + removeOptimistic( + recordingTargetRef.current?.region ?? currentRegionRef.current + ); + // The take is gone, so the clause it was held against is released too. + latchRecordingTarget(undefined); setPhase('recordReady'); setCurrentClausePlayed(true); setResetMedia(true); @@ -1787,34 +1850,19 @@ 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); /** - * 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. + * Disable only the Record button until expected clause playback time expires. * - * 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. + * 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). * - * 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. - * - * 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(); @@ -1911,6 +1959,7 @@ export function PassageDetailGuidedPhraseRecord({ onPlayStatusNotify={handlePlayStatusNotify} beforePlay={handleBeforeSourcePlay} lockSegmentSelection={segmentSelectionLocked} + isSegmentRecorded={isSegmentRecorded} allowZoom={true} /> )} @@ -1931,13 +1980,13 @@ export function PassageDetailGuidedPhraseRecord({ canSplitClause={canSplitClause( currentIndex, clauseRegions, - completedIndices, + recordedClauseIndicesForTools, currentClauseSplitPoint )} canCombineWithNext={canCombineWithNext( currentIndex, clauseRegions, - completedIndices + recordedClauseIndicesForTools )} showUndoCombine={ combineUndo !== null && !config.multiLevelSegmentUndo @@ -1963,7 +2012,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} @@ -1971,12 +2020,25 @@ export function PassageDetailGuidedPhraseRecord({ onRecording={(active) => { if (active) { recordingActiveRef.current = true; + // 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, + region: currentRegion, + }); + } // A new take supersedes any earlier rejected save (TT-7583). saveRejectedRef.current = false; setSaveRejected(false); // 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; @@ -2002,8 +2064,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); + // Also clear optimistic green on this failure path (TT-7583). + // Keep the latch so Retry still files to the same clause. + removeOptimistic( + recordingTargetRef.current?.region ?? currentRegionRef.current + ); applyColors(); }} setStatusText={setStatusText} diff --git a/src/renderer/src/components/PassageDetail/PassageDetailPlayer.tsx b/src/renderer/src/components/PassageDetail/PassageDetailPlayer.tsx index 2d78dee9e..fafe6b69a 100644 --- a/src/renderer/src/components/PassageDetail/PassageDetailPlayer.tsx +++ b/src/renderer/src/components/PassageDetail/PassageDetailPlayer.tsx @@ -118,6 +118,8 @@ export interface DetailPlayerProps { beforePlay?: () => void | Promise; /** When true, waveform region clicks cannot change the selected segment. */ lockSegmentSelection?: boolean; + /** 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). */ showTranscriptionButton?: boolean; @@ -130,6 +132,7 @@ export function PassageDetailPlayer(props: DetailPlayerProps) { allowSegment, allowAutoSegment, hideSegmentControls, + isSegmentRecorded, saveSegments, suggestedSegments, forceRegionOnly, @@ -454,6 +457,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/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..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,15 +71,27 @@ 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; } +/** + * 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) => regionsMatch(c, region, tolerance)); +} + /** @deprecated Prefer hasPhraseRegions — kept for BOLD clause naming at call sites. */ export const hasClauseRegions = hasPhraseRegions; 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(); } diff --git a/src/renderer/src/components/WSAudioPlayer.tsx b/src/renderer/src/components/WSAudioPlayer.tsx index 9c19b2bbe..22bfa7449 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] ); + // If a segment is recorded, disable +/- when they would change its boundary + // (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..d2c3e6a9e 100644 --- a/src/renderer/src/components/WSAudioPlayerSegment.tsx +++ b/src/renderer/src/components/WSAudioPlayerSegment.tsx @@ -120,13 +120,19 @@ function WSAudioPlayerSegment(props: IProps) { const handleShowSettings = () => { setShowSettings(!showSettings); }; + // Keep Add disabled state and styling in sync. + // disableSplit covers boundary and recorded-segment cases. + const splitDisabled = !ready || busyRef.current || !!disableSplit; + const handleSplit = () => { if (!readyRef.current) return false; if (setBusy) setBusy(true); 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; @@ -135,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, @@ -199,8 +206,9 @@ 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..90820b517 --- /dev/null +++ b/src/renderer/src/components/WSAudioPlayerSegmentRecordedLock.test.ts @@ -0,0 +1,66 @@ +import { + isAddBlockedByRecording, + isRemoveBlockedByRecording, +} from './segmentBoundaryLocks'; +import { IRegion } from '../crud/useWavesurferRegions'; + +/** + * Tests for +/- blocking rules on recorded segments (TT-7666). + */ + +// Three contiguous 10-second 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; + // 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; + // 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 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; + // 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; + // 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 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 new file mode 100644 index 000000000..e377cb28a --- /dev/null +++ b/src/renderer/src/components/segmentBoundaryLocks.ts @@ -0,0 +1,55 @@ +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. Assumes sorted input. */ +export function segmentIndexAtPlayhead( + progressSec: number, + regions: IRegion[] +): number { + for (let i = 0; i < regions.length; i++) { + const isLast = i === regions.length - 1; + if ( + progressSec >= regions[i].start && + (isLast ? progressSec <= regions[i].end : progressSec < regions[i].end) + ) { + return i; + } + } + return -1; +} + +/** Block Add when it would split a recorded segment (TT-7666). */ +export function isAddBlockedByRecording( + progressSec: number, + regions: IRegion[], + isSegmentRecorded?: (index: number) => boolean +): boolean { + if (!isSegmentRecorded) return false; + const idx = segmentIndexAtPlayhead(progressSec, regions); + return idx >= 0 && isSegmentRecorded(idx); +} + +/** Block Remove when either side of the merged boundary is recorded (TT-7666). + * Assumes sorted input. */ +export function isRemoveBlockedByRecording( + progressSec: number, + regions: IRegion[], + tol: number, + isSegmentRecorded?: (index: number) => boolean +): boolean { + if (!isSegmentRecorded || regions.length < 2) return false; + for (let i = 0; i < regions.length - 1; i++) { + if (Math.abs(progressSec - regions[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 new file mode 100644 index 000000000..c5a68cbf8 --- /dev/null +++ b/src/renderer/src/crud/useWavesurferRegions.test.tsx @@ -0,0 +1,580 @@ +import { act, renderHook } from '@testing-library/react'; + +/** + * Segment lock spec (TT-7437). + * + * 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 ----------------------------------------- +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 finds plugins with `instanceof RegionsPlugin`, so this fake must be +// that 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(params: any) { + // wavesurfer returns a Region object and the hook 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; + } + 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; + /** Playhead position used by split tests. */ + progressAt?: number; + /** Sorted indices of recorded segments (TT-7666). */ + recordedIndices?: number[]; + /** Provide a color function so applyRegionColors runs. */ + withColors?: boolean; +} + +const renderRegions = ({ + lockSegmentSelection, + progressAt = 0, + recordedIndices = [], + withColors = false, +}: 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(); + + // 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, + segs, + onCurrentRegion, + onRegionClicked, + goto, + setPlaying, + update, + }; +}; + +// The user taps a segment on the waveform. +const clickSegment = (plugin: IFakePlugin, r: any) => + act(() => { + plugin.emit('region-clicked', r, { stopPropagation: jest.fn() }); + }); + +// Playhead enters a segment (for example, after a tap seeks). +const playheadEnters = (plugin: IFakePlugin, r: any) => + act(() => { + plugin.emit('region-in', r); + }); + +// Drag a segment boundary and finish the drag. +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 + ); + }); + + 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)', () => { + 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', () => { + // A tap can still seek and emit region-in. Lock must block that path too. + 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 reshape the active take's segment. + 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('does not split a segment on double-click', () => { + // Double-click split also changes boundaries, so it must be blocked. + 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 }); + + 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 + }); + + 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)', () => { + // Recorded boundaries are frozen (TT-7666). Since a boundary is shared, + // drag is blocked when either neighboring segment is recorded. + + 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({ + lockSegmentSelection: false, + recordedIndices: [2], + }); + + dragBoundary(plugin, segs[1], 'start', 8); + + expect(onCurrentRegion).toHaveBeenCalledWith( + expect.objectContaining({ start: 8 }), + 1 + ); + }); + + it('freezes a recorded segment but keeps its boundary handles visible', () => { + // Keep the handle visible, but disable dragging on both sides (TT-7666). + const { segs } = renderRegions({ + lockSegmentSelection: false, + recordedIndices: [1], + }); + + expect(segs[1].setOptions).toHaveBeenCalledWith( + 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 }) + ); + }); + + 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], + withColors: true, + }); + segs.forEach((s) => s.setOptions.mockClear()); + + act(() => { + result.current.applyRegionColors(); + }); + + // 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: 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 }) + ); + }); +}); + +describe('useWaveSurferRegions — the +/- controls on a recorded segment (TT-7666)', () => { + // 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({ + 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 44d675568..29f42a3b6 100644 --- a/src/renderer/src/crud/useWavesurferRegions.tsx +++ b/src/renderer/src/crud/useWavesurferRegions.tsx @@ -126,18 +126,20 @@ 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, /** * 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 a sorted segment index already has a recording (TT-7666). + * Boundaries shared with recorded segments are not draggable. + */ + isSegmentRecorded?: (sortedIndex: number) => boolean ) { const theme = useTheme(); const wsRef = useRef(ws); @@ -164,6 +166,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). */ @@ -193,9 +197,14 @@ export function useWaveSurferRegions( applyRegionColorRef.current = applyRegionColor; }, [applyRegionColor]); + // 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; - }, [lockSegmentSelection]); + applyBoundaryEditability(); + // applyBoundaryEditability reads refs; deps are the two reactive inputs. + // eslint-disable-next-line react-hooks/exhaustive-deps + }, [lockSegmentSelection, isSegmentRecorded]); useEffect(() => { disableDragSelectionRef.current = disableDragSelection ?? false; @@ -238,6 +247,56 @@ export function useWaveSurferRegions( r.setOptions({ color: base }); } }); + applyBoundaryEditability(); + }; + + /** Show ew-resize only on draggable handles; otherwise show default cursor. */ + 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'; + }; + + /** + * 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; + // Players without lock/recorded checks keep default region behavior. + if (!locked && !recorded) return; + const sorted = sortedRegions(); + const last = sorted.length - 1; + sorted.forEach((r, i) => { + let startActive: boolean; + let endActive: boolean; + if (locked) { + // Recording in progress: nothing moves. + startActive = false; + endActive = false; + } else { + // 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 handles visible; gate side drag via resizeStart/resizeEnd. + r.setOptions({ + resize: true, + resizeStart: startActive, + resizeEnd: endActive, + }); + setHandleCursor(r, 'left', startActive); + setHandleCursor(r, 'right', endActive); + }); }; const Regions = () => regionsRef.current; @@ -322,6 +381,8 @@ 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 split is blocked during recording lock (TT-7437). + if (lockSegmentSelectionRef.current) return; const currentTime = getCurrentTime(); const timeSinceLastDoubleClick = currentTime - lastDoubleClickTimeRef.current; @@ -420,11 +481,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); @@ -496,6 +554,11 @@ export function useWaveSurferRegions( regionsPlugin.on('region-created', function (r: Region) { if (isMarker(r)) return; r.drag = singleRegionRef.current; + // 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 }); + } // Round region start and end to 5 decimal places because the seek uses 5 decimal places r.start = roundToFiveDecimals(r.start); @@ -562,6 +625,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 +638,9 @@ export function useWaveSurferRegions( regionsPlugin.on( 'region-updated', function (r: Region, side?: UpdateSide) { + // region-updated can change selection and boundaries, so block it + // while recording lock is active (TT-7437). + if (lockSegmentSelectionRef.current) return; if (singleRegionRef.current) { if (!loadingRef.current) { waitForIt( @@ -614,16 +681,15 @@ 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. + // 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); }); 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 && @@ -685,6 +751,8 @@ export function useWaveSurferRegions( color: 'rgba(255, 0, 0, 0.1)', }); } + // Apply recorded-boundary locks for existing segments (TT-7666). + applyBoundaryEditability(); } }; @@ -770,17 +838,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; @@ -974,6 +1033,8 @@ export function useWaveSurferRegions( region.id = r?.id; }); setPrevNext(regarray.map((r: any) => r.id)); + // Re-apply recorded-boundary locks after loading regions (TT-7666). + applyBoundaryEditability(); onRegion(regarray.length, newRegions); onRegionGoTo(regarray[defaultRegionIndex]?.start ?? 0); loadingRef.current = false; @@ -1069,12 +1130,31 @@ 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) { clearRegions(); return; } + // 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); + 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, @@ -1122,7 +1202,15 @@ export function useWaveSurferRegions( }; const wsAddRegion = () => { - return wsSplitRegion(findRegion(progress(), true), progress()); + // 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))) { + return undefined; + } + return wsSplitRegion(target, progress()); }; const wsRemoveCurrentRegion = () => {