diff --git a/apps/web/core/debates/browse/debate-feed-player.test.tsx b/apps/web/core/debates/browse/debate-feed-player.test.tsx new file mode 100644 index 0000000000..f7a923b54f --- /dev/null +++ b/apps/web/core/debates/browse/debate-feed-player.test.tsx @@ -0,0 +1,164 @@ +import { render } from '@testing-library/react'; + +import { beforeEach, describe, expect, it, vi } from 'vitest'; + +import type { Debate, DebateParticipant } from '~/core/debates/api'; +import type { DebateVotesResult } from '~/core/debates/use-debate-votes'; + +import { DebateFeedPlayer } from './debate-feed-player'; + +const mocks = vi.hoisted(() => ({ controller: null as unknown })); + +vi.mock('~/core/debates/use-debate-playback', () => ({ + useDebatePlayback: () => mocks.controller, +})); + +vi.mock('~/core/debates/use-playback-analytics', () => ({ + usePlaybackAnalytics: () => ({ elementRef: { current: null }, control: vi.fn() }), +})); + +vi.mock('~/core/hooks/use-entity-side-panel', () => ({ + useEntitySidePanel: () => ({ openSidePanel: vi.fn(), closeSidePanel: vi.fn(), sidePanelTarget: null }), +})); + +vi.mock('~/core/hooks/use-space', () => ({ + useSpace: () => ({ space: null }), +})); + +const participant = (slot: 1 | 2): DebateParticipant => + ({ + participant_slot: slot, + profile_space_id: `space-${slot}`, + position_label: slot === 1 ? 'For' : 'Against', + }) as unknown as DebateParticipant; + +const votes: DebateVotesResult = { + sharePercentFor: () => null, + isMyPick: () => false, + hasVoted: false, + isVoting: false, + castVote: async () => {}, +}; + +/** + * A controller in the one state that matters here: playing, with a turn in progress, both + * recordings loaded. `mutedByUser` and `turnState` are what the audio gating reads. + */ +function controllerFixture(overrides: { mutedByUser: boolean; turnSlot: 1 | 2; isResuming?: boolean }) { + return { + slot1VideoRef: { current: null }, + slot2VideoRef: { current: null }, + slot1Participant: participant(1), + slot2Participant: participant(2), + urls: { slot1: 'https://cdn.test/slot1.webm', slot2: 'https://cdn.test/slot2.webm' }, + ready: true, + error: null, + playing: true, + userPaused: false, + isScrubbing: false, + isResuming: overrides.isResuming ?? false, + playbackEnded: false, + mutedByUser: overrides.mutedByUser, + setMutedByUser: vi.fn(), + playheadSeconds: 5, + timelineSeconds: 60, + turnState: { slot: overrides.turnSlot, seconds: 10, progress: 0.5 }, + activeSlot: overrides.turnSlot, + subtitle: null, + onPlaybackTick: vi.fn(), + togglePlayback: vi.fn(), + playFromStart: vi.fn(), + resumeBoth: vi.fn(), + suspend: vi.fn(), + seekBoth: vi.fn(), + beginScrub: vi.fn(), + endScrub: vi.fn(), + }; +} + +function renderPlayer(overrides: { mutedByUser: boolean; turnSlot: 1 | 2; isResuming?: boolean }) { + mocks.controller = controllerFixture(overrides); + const { container, rerender } = render( + + ); + const [slot1, slot2] = Array.from(container.querySelectorAll('video')); + return { + slot1, + slot2, + /** Re-render with a new controller state, as the hook's own state changes would. */ + update(next: { mutedByUser: boolean; turnSlot: 1 | 2; isResuming?: boolean }) { + mocks.controller = controllerFixture(next); + rerender(); + }, + }; +} + +/** + * Per-turn audio is the `muted` flag: only the debater whose turn it is is audible, and the + * viewer's own mute wins over both. Unchanged from master — an attempt to move this gate onto + * `volume` was reverted, since Blink's effective mute is `muted || volume === 0`, so a listening + * element at volume 0 is exactly as stoppable off screen as a muted one. + */ +describe('DebateFeedPlayer audio gating (GEO-2947)', () => { + beforeEach(() => { + mocks.controller = null; + }); + + it('leaves only the speaking debater audible once the viewer un-mutes', () => { + const { slot1, slot2 } = renderPlayer({ mutedByUser: false, turnSlot: 1 }); + + expect(slot1.muted).toBe(false); + expect(slot2.muted).toBe(true); + }); + + it('moves the mute with the turn', () => { + const { slot1, slot2 } = renderPlayer({ mutedByUser: false, turnSlot: 2 }); + + expect(slot1.muted).toBe(true); + expect(slot2.muted).toBe(false); + }); + + it("honours the viewer's mute on both elements", () => { + const { slot1, slot2 } = renderPlayer({ mutedByUser: true, turnSlot: 1 }); + + expect(slot1.muted).toBe(true); + expect(slot2.muted).toBe(true); + }); +}); + +describe('DebateFeedPlayer repairs a mute made behind React (GEO-2947)', () => { + it('re-asserts the rendered mute once the resume is over', () => { + const { slot1, update } = renderPlayer({ mutedByUser: false, turnSlot: 1, isResuming: true }); + + // What the fallback does to the speaking element mid-retry, behind React's back. + slot1.muted = true; + expect(slot1.muted).toBe(true); + + update({ mutedByUser: false, turnSlot: 1, isResuming: false }); + + expect(slot1.muted).toBe(false); + }); + + /** And it must not do it *during* the retry — that would block the play it is waiting on. */ + it('leaves the mute alone while the resume is still confirming', () => { + const { slot1, update } = renderPlayer({ mutedByUser: false, turnSlot: 1, isResuming: true }); + + slot1.muted = true; + // A tick mid-attempt: the turn moves, so the component re-renders and the effect re-runs. + update({ mutedByUser: false, turnSlot: 2, isResuming: true }); + + expect(slot1.muted).toBe(true); + }); + + /** A mute the viewer made during the attempt wins — it is the rendered truth by then. */ + it('repairs to the newer preference, not the one the attempt started with', () => { + const { slot1, slot2, update } = renderPlayer({ mutedByUser: false, turnSlot: 1, isResuming: true }); + + slot1.muted = true; + slot2.muted = true; + update({ mutedByUser: true, turnSlot: 1, isResuming: false }); + + expect(slot1.muted).toBe(true); + expect(slot2.muted).toBe(true); + }); +}); diff --git a/apps/web/core/debates/browse/debate-feed-player.tsx b/apps/web/core/debates/browse/debate-feed-player.tsx index 077215b8d0..57e0c83d8c 100644 --- a/apps/web/core/debates/browse/debate-feed-player.tsx +++ b/apps/web/core/debates/browse/debate-feed-player.tsx @@ -51,6 +51,7 @@ export function DebateFeedPlayer({ debate, active, preload = false, votes }: Deb playing, userPaused, isScrubbing, + isResuming, playbackEnded, mutedByUser, setMutedByUser, @@ -108,6 +109,7 @@ export function DebateFeedPlayer({ debate, active, preload = false, votes }: Deb countdown={playing && turnState?.slot === 1 ? turnState : null} subtitle={activeSlot === 1 ? subtitle : null} mutedByUser={mutedByUser} + isResuming={isResuming} onPlaybackTick={onPlaybackTick} onToggle={togglePlayback} votes={votes} @@ -145,6 +147,7 @@ export function DebateFeedPlayer({ debate, active, preload = false, votes }: Deb countdown={playing && turnState?.slot === 2 ? turnState : null} subtitle={activeSlot === 2 ? subtitle : null} mutedByUser={mutedByUser} + isResuming={isResuming} onPlaybackTick={onPlaybackTick} onToggle={togglePlayback} votes={votes} @@ -200,6 +203,7 @@ function DebaterVideo({ countdown, subtitle, mutedByUser, + isResuming, onPlaybackTick, onToggle, votes, @@ -213,6 +217,7 @@ function DebaterVideo({ countdown: TurnState; subtitle: string | null; mutedByUser: boolean; + isResuming: boolean; onPlaybackTick: () => void; onToggle: () => void; votes: DebateVotesResult; @@ -222,6 +227,31 @@ function DebaterVideo({ const { openSidePanel } = useEntitySidePanel(); const name = participant ? speakerLabel(participant) : 'Debater'; + const muted = !audible || mutedByUser; + + /** + * Re-assert the rendered mute after a resume (GEO-2947). + * + * `playBothWithMutedFallback` mutes both elements to retry a blocked play and cannot put them + * back: it does not know what this component renders `muted` from, and anything it captured is + * a confirm window out of date by the time it could write it. React will not repair that either + * — it writes a DOM property only when its *own* previous value differs, and a mute it never + * made is invisible to it — so the element would play audibly under a UI showing muted. + * + * Not while a resume is confirming: the retry depends on the mute it just made, and writing over + * it mid-attempt would block the play this is trying to let happen. `isResuming` falling is + * itself what runs this effect again, so the repair lands the moment the attempt is over. + * + * This is why `playFromStart` no longer writes `muted` either. The value it has (`mutedByUser`) + * is not the value rendered here, so repairing from the hook moved the divergence rather than + * closing it. `muted` has one owner: this render. + */ + React.useLayoutEffect(() => { + const video = videoRef.current; + if (!video || isResuming) return; + video.muted = muted; + }, [isResuming, muted, src, videoRef]); + // A personal space's own id resolves to its "system entity" (an ugly technical // record). The space's page entity is the real profile, so open that once it's // loaded and fall back to the space id while it's still fetching. @@ -244,7 +274,8 @@ function DebaterVideo({ playsInline preload="metadata" src={src} - muted={!audible || mutedByUser} + // The viewer's own mute — plus the listening debater's, where `volume` is a no-op. + muted={muted} onEnded={onPlaybackTick} onLoadedMetadata={onPlaybackTick} onPause={onPlaybackTick} diff --git a/apps/web/core/debates/playback-utils.test.ts b/apps/web/core/debates/playback-utils.test.ts index 7e16ef715d..9fd34db830 100644 --- a/apps/web/core/debates/playback-utils.test.ts +++ b/apps/web/core/debates/playback-utils.test.ts @@ -5,6 +5,7 @@ import { clampSeconds, hasProcessedVideo, normalizeTurnDurationsMs, + pairPlayhead, playBothWithMutedFallback, recordingWindowOffsetsSeconds, sortTurnSegments, @@ -236,6 +237,116 @@ describe('recordingWindowOffsetsSeconds', () => { }); }); +describe('pairPlayhead (GEO-2947)', () => { + const offsets = { slot1: 1, slot2: 3 }; + const video = (paused: boolean, currentTime: number) => ({ paused, currentTime }); + + it('reads slot 1 while it is running', () => { + expect(pairPlayhead(video(false, 10), video(false, 8), offsets)).toEqual({ seconds: 11, live: true }); + }); + + it('falls back to slot 1 when both are paused', () => { + expect(pairPlayhead(video(true, 10), video(true, 8), offsets)).toEqual({ seconds: 11, live: false }); + }); + + /** + * In the foreground slot 1 is canonical, full stop. Slot 2 is *deliberately* allowed to run + * ahead — the drift nudge puts it there, and a stalled slot 1 leaves it much further ahead + * (GEO-2828) — so an ordinary pause or a scroll-away must not resume from it, or playback skips + * audio the viewer never heard. + */ + describe('without trustSecondary (the foreground)', () => { + it('ignores a slot 2 that has run ahead of a paused slot 1', () => { + expect(pairPlayhead(video(true, 10), video(true, 30), offsets)).toEqual({ seconds: 11, live: false }); + }); + + it('ignores a running slot 2 while slot 1 is paused', () => { + expect(pairPlayhead(video(true, 10), video(false, 30), offsets)).toEqual({ seconds: 11, live: false }); + }); + + it('still lets the remembered position raise a frozen slot 1', () => { + expect(pairPlayhead(video(true, 10), video(true, 30), offsets, 20)).toEqual({ seconds: 20, live: false }); + }); + }); + + /** + * Off screen the picture inverts: the browser stops whichever element it considers silent, so + * slot 1's clock can be frozen at an arbitrary past instant while slot 2 carries the debate on. + */ + describe('with trustSecondary (a backgrounded tab)', () => { + it('reads the element still running when slot 1 is the one that stopped', () => { + // Slot 1 frozen at 10 (debate 11) while slot 2 has reached 20 (debate 23). + expect(pairPlayhead(video(true, 10), video(false, 20), offsets, null, true)).toEqual({ + seconds: 23, + live: true, + }); + }); + + /** + * The sequence with no tick in between: slot 1 stops, slot 2 plays on, slot 2 stops too. + * `timeupdate` is throttled in a background tab, so slot 2 stops somewhere the record never + * saw, and its own `pause` arrives when it already reads as paused. Its frozen clock is the + * only evidence left. + */ + it('weighs slot 2 frozen clock when it stopped past the last position recorded', () => { + expect(pairPlayhead(video(true, 10), video(true, 30), offsets, 23, true)).toEqual({ + seconds: 33, + live: false, + }); + }); + + /** + * ...but only once slot 2 has actually played. A recording that starts after the debate + * window has a positive offset, so an untouched slot 2 would otherwise read as being that far + * into the debate. + */ + it('ignores a slot 2 that has never played, offset and all', () => { + expect(pairPlayhead(video(true, 0), video(true, 0), offsets, null, true)).toEqual({ + seconds: 1, + live: false, + }); + }); + + /** + * THE RATCHET. A browser that stopped slot 1 at debate-time 100 while slot 2 ran on to 130 can + * hand slot 1 back un-paused at its frozen 100 — un-suspended, or un-paused-but-stalled, which + * `paused === false` cannot tell apart. Reading that as "live, we are at 100" walks the + * recovered position backwards over half a minute the viewer already heard, and the resume + * drags slot 2 back with it. + */ + it('does not let a running slot 1 walk the position back over a record ahead of it', () => { + const slot1 = video(false, 99); // running, frozen behind: debate-time 100 + const slot2 = video(true, 127); // stopped at debate-time 130 + expect(pairPlayhead(slot1, slot2, offsets, 130, true)).toEqual({ seconds: 130, live: true }); + }); + + /** Nor over the other element's clock, when there is no record to fall back on. */ + it('does not let a running slot 1 walk the position back over a stopped slot 2', () => { + expect(pairPlayhead(video(false, 99), video(true, 127), offsets, null, true)).toEqual({ + seconds: 130, + live: true, + }); + }); + + it('keeps the frozen clock when it is ahead of the remembered position', () => { + expect(pairPlayhead(video(true, 40), video(true, 5), offsets, 12, true)).toEqual({ + seconds: 41, + live: false, + }); + }); + }); + + /** In the foreground a running slot 1 always wins over the record — that keeps a scrub honest. */ + it('ignores the remembered position while slot 1 is running', () => { + expect(pairPlayhead(video(false, 5), video(true, 20), offsets, 90)).toEqual({ seconds: 6, live: true }); + }); + + it('survives an element that is not mounted yet', () => { + expect(pairPlayhead(null, null, offsets)).toEqual({ seconds: 1, live: false }); + expect(pairPlayhead(null, video(false, 20), offsets, null, true)).toEqual({ seconds: 23, live: true }); + }); +}); + describe('playBothWithMutedFallback (GEO-2783)', () => { /** * A fake video whose `play()` refuses while it has audio, which is what the autoplay policy @@ -288,7 +399,7 @@ describe('playBothWithMutedFallback (GEO-2783)', () => { b.tick(); }; - expect(await playBothWithMutedFallback(a, b, wait)).toBe('playing'); + expect(await playBothWithMutedFallback(a, b, { wait })).toBe('playing'); // The point of the fix: no muted retry, because it was never actually blocked. expect([a.plays, b.plays]).toEqual([1, 1]); }); @@ -302,7 +413,7 @@ describe('playBothWithMutedFallback (GEO-2783)', () => { }; // Both already muted, so there is no fallback to try — this must not be reported as playing. - expect(await playBothWithMutedFallback(a, b, wait)).toBe('blocked'); + expect(await playBothWithMutedFallback(a, b, { wait })).toBe('blocked'); }); it('plays straight away when the browser allows it', async () => { @@ -351,6 +462,73 @@ describe('playBothWithMutedFallback (GEO-2783)', () => { expect(await playBothWithMutedFallback(a, b)).toBe('blocked'); }); + /** + * A mute left behind by a failed retry is invisible to React — it only writes a DOM property + * when its own previous value differs — so it survives every later render that says otherwise, + * leaving the pair silently muted under a UI still offering a "mute" control (GEO-2947). + */ + it('leaves the pair muted when it gives up, for the renderer to repair', async () => { + // Refuses to start either way, so the muted retry fails too and the helper gives up. + const stuck = () => { + const video = fakeVideo({ muted: false }); + video.play = async () => { + video.plays += 1; + }; + return video; + }; + const a = stuck(); + const b = stuck(); + + expect(await playBothWithMutedFallback(a, b)).toBe('blocked'); + + // Deliberately not restored. Anything captured on the way in is up to ~300ms stale by now — + // the viewer can mute mid-attempt — and React never repairs a DOM write it did not make, so + // a stale `false` written back here would play audibly under a UI showing muted (GEO-2947). + expect(a.muted).toBe(true); + expect(b.muted).toBe(true); + }); + + /** ...but a retry that *worked* keeps the mute: the caller records it as the rendered truth. */ + it('keeps the mute when the muted retry succeeds', async () => { + const a = fakeVideo({ muted: false, blockUnmuted: true }); + const b = fakeVideo({ muted: false, blockUnmuted: true }); + + expect(await playBothWithMutedFallback(a, b)).toBe('playing-muted'); + + expect(a.muted).toBe(true); + expect(b.muted).toBe(true); + }); + + /** + * The retry is the only point where this function starts something it did not start, and it is + * reached after a confirm window it spent asleep — so a pause or a scroll-away routinely lands + * in between. A caller that checks ownership only once this returns is too late: `play()` has + * already been called, and no state check can take it back. + */ + it('does not retry when the attempt was cancelled while confirming', async () => { + const a = fakeVideo({ muted: false }); + const b = fakeVideo({ muted: false }); + + expect(await playBothWithMutedFallback(a, b, { isCancelled: () => true })).toBe('cancelled'); + + expect(a.plays).toBe(1); // the first attempt only — no restart behind the viewer's pause + expect(b.plays).toBe(1); + expect(a.paused).toBe(true); + expect(b.paused).toBe(true); + expect(a.muted).toBe(false); // and nothing was force-muted for a retry that never ran + }); + + /** The control: an attempt nobody superseded still retries muted and reports it. */ + it('still retries when nothing cancelled it', async () => { + const a = fakeVideo({ muted: false }); + const b = fakeVideo({ muted: false }); + + expect(await playBothWithMutedFallback(a, b, { isCancelled: () => false })).toBe('playing-muted'); + + expect(a.plays).toBe(2); + expect(a.muted).toBe(true); + }); + it('keeps sound when the play is gesture-driven', async () => { const a = fakeVideo({ muted: false, blockUnmuted: false }); const b = fakeVideo({ muted: false, blockUnmuted: false }); diff --git a/apps/web/core/debates/playback-utils.ts b/apps/web/core/debates/playback-utils.ts index 5478a34783..6bbba3ad3a 100644 --- a/apps/web/core/debates/playback-utils.ts +++ b/apps/web/core/debates/playback-utils.ts @@ -147,6 +147,84 @@ function offsetSeconds(startedAtMs: number | null, windowStartMs: number): numbe return (startedAtMs - windowStartMs) / 1_000; } +/** What `pairPlayhead` needs off an element, so tests need not build a whole video. */ +export type ClockVideo = Pick; + +export type PairPlayhead = { + /** Debate-timeline seconds. */ + seconds: number; + /** Read off an element that is actually running, rather than reconstructed from a paused one. */ + live: boolean; +}; + +/** + * Where the debate is, in debate-timeline seconds, given the two recordings' own clocks. + * + * Slot 1 is the clock, always, in the foreground. The pair is kept in lockstep, so its position + * plus its recording offset is the debate's position, and every seek is expressed that way. Slot 2 + * is *not* interchangeable with it: it is deliberately allowed to run ahead — the drift nudge puts + * it there on purpose, and a stalled slot 1 can leave it far ahead (GEO-2828) — so treating its + * clock as the debate's would skip whatever it had got through but the viewer had not heard. + * + * `trustSecondary` is what turns that off, and it means one specific thing: the pair was stopped + * by the browser rather than by us, which happens when the tab is off screen (GEO-2947). There, + * slot 1's clock is not canonical at all — it is frozen at whatever instant the browser stopped + * the element it considered silent, while slot 2 carried the debate on — so the position has to + * be recovered from whatever evidence there is: + * + * - whichever element is still running, slot 1 first; + * - then the furthest frozen clock, since a browser does not stop both at the same instant; + * - and `lastRunningSeconds`, the caller's own record, which covers the case where an element + * stopped between the throttled `timeupdate` ticks of a background tab. + * + * Slot 2's frozen clock counts only once its `currentTime` shows the recording has played: one + * that starts after the debate window has a positive offset, so an untouched slot 2 would + * otherwise report being that far into the debate before a frame of it had been shown. + * + * The caller must reset its record on a deliberate seek and when the recordings change, or a + * scrub backwards would be dragged forward by it — `useDebatePlayback` does both. + * + * `live` says whether the answer came off a running element, so a caller can keep its record + * current without re-deriving "is either element running" for itself. + */ +export function pairPlayhead( + primary: ClockVideo | null, + secondary: ClockVideo | null, + offsets: { slot1: number; slot2: number }, + lastRunningSeconds: number | null = null, + trustSecondary = false +): PairPlayhead { + const primarySeconds = (primary?.currentTime ?? 0) + offsets.slot1; + const primaryRunning = Boolean(primary && !primary.paused); + + if (!trustSecondary) { + // Slot 1 is the clock and a running slot 1 is the whole answer: the record is only there to + // raise a *frozen* one. Anything else would drag a scrub backwards forward again. + if (primaryRunning) return { seconds: primarySeconds, live: true }; + return { + seconds: lastRunningSeconds === null ? primarySeconds : Math.max(primarySeconds, lastRunningSeconds), + live: false, + }; + } + + // Off screen no clock is authoritative, *including a running one*. A browser that stopped slot 1 + // at debate-time 100 while slot 2 ran on to 130 can hand slot 1 back un-paused at its frozen 100 + // — un-suspended, or un-paused-but-stalled, which `paused === false` cannot tell apart and which + // is slot 1's documented failure mode (GEO-2828). Returning that 100 as "live" would walk the + // recovered position backwards over half a minute the viewer had already heard, and drag slot 2 + // back with it on the resume. So every piece of evidence is weighed and the furthest wins. + const candidates = [primarySeconds]; + // Slot 2 counts once its clock shows the recording has played: one that starts after the debate + // window has a positive offset, so an untouched slot 2 would otherwise report being that far in. + if (secondary && secondary.currentTime > 0) candidates.push(secondary.currentTime + offsets.slot2); + if (lastRunningSeconds !== null) candidates.push(lastRunningSeconds); + + return { + seconds: Math.max(...candidates), + live: primaryRunning || Boolean(secondary && !secondary.paused), + }; +} + export function participantForSlot(debate: Debate, slot: ParticipantSlot) { return debate.participants.find(participant => participant.participant_slot === slot) ?? null; } @@ -162,7 +240,23 @@ export function speakerLabel(participant: Pick & { play: () => Promise }; -export type PlayBothOutcome = 'playing' | 'playing-muted' | 'blocked'; +export type PlayBothOutcome = 'playing' | 'playing-muted' | 'blocked' | 'cancelled'; + +export type PlayBothOptions = { + /** Injectable so tests do not wait on real timers. */ + wait?: (ms: number) => Promise; + /** + * Has something else taken ownership of these elements since the attempt began? + * + * This function confirms a start by polling, so it is *asleep* for most of its runtime, and a + * pause or a scroll-away lands there routinely. A caller that only checks ownership once this + * returns is too late: the retry below would have called `play()` on both elements in the + * meantime, leaving a pair the viewer had paused running in the DOM under a UI showing paused + * (GEO-2947). Checked before the retry, which is the only point where this function restarts + * something it did not start. + */ + isCancelled?: () => boolean; +}; /** * Start both recordings, falling back to muted when the browser blocks unmuted autoplay @@ -209,8 +303,7 @@ async function bothRunning( export async function playBothWithMutedFallback( primary: PlayableVideo, secondary: PlayableVideo, - /** Injectable so tests do not wait on real timers. */ - wait: (ms: number) => Promise = defaultWait + { wait = defaultWait, isCancelled }: PlayBothOptions = {} ): Promise { const attempt = async () => { await Promise.allSettled([primary.play(), secondary.play()]); @@ -219,9 +312,25 @@ export async function playBothWithMutedFallback( if (await attempt()) return 'playing'; + // Someone paused these, or scrolled them off screen, while the confirm above was polling. The + // retry would start them again — and the caller checking ownership after this returns cannot + // undo a `play()` that has already happened. + if (isCancelled?.()) return 'cancelled'; + // Nothing to retry if audio was already off — the block is not the autoplay policy. if (primary.muted && secondary.muted) return 'blocked'; + // Both outcomes below leave the elements muted, and deliberately so: this function does not + // know what the caller renders `muted` from, and it has been awaiting for up to ~300ms, so any + // value it captured on the way in may already be out of date — the viewer can mute from the + // control that stays visible during playback, or from another card sharing the preference. + // Writing a stale snapshot back is worse than leaving the mute: React only writes a DOM + // property when its own previous value differs, so a write it disagrees with is one it will + // never repair, and the pair would play audibly under a UI showing muted (GEO-2947). + // + // 'playing-muted' is paired by the caller with the state change that makes the mute the + // rendered truth. 'blocked' is repaired by whoever renders `muted`, once the attempt is over — + // `DebateFeedPlayer` re-asserts it when `isResuming` falls. primary.muted = true; secondary.muted = true; return (await attempt()) ? 'playing-muted' : 'blocked'; diff --git a/apps/web/core/debates/use-debate-playback.test.tsx b/apps/web/core/debates/use-debate-playback.test.tsx index e3e596c566..36120506d6 100644 --- a/apps/web/core/debates/use-debate-playback.test.tsx +++ b/apps/web/core/debates/use-debate-playback.test.tsx @@ -1,6 +1,6 @@ import { act, renderHook, waitFor } from '@testing-library/react'; -import { beforeEach, describe, expect, it, vi } from 'vitest'; +import { afterEach, beforeEach, describe, expect, it, vi } from 'vitest'; import type { Debate, DebateMediaTurnSegment } from './api'; import { useDebatePlayback } from './use-debate-playback'; @@ -123,7 +123,14 @@ function fakeVideo() { currentTime: 0, playbackRate: 1, pending: null as null | { resolve: () => void; reject: (reason: unknown) => void }, + /** + * How many times playback has been asked for. The interesting question for a cancelled + * attempt is not whether the element ends up paused — a pause that lands after the restart + * leaves it paused either way — but whether anything asked it to start again at all. + */ + plays: 0, play() { + video.plays += 1; return new Promise((resolve, reject) => { video.pending = { resolve, reject }; }); @@ -148,8 +155,30 @@ function fakeVideo() { video.pending.resolve(); video.pending = null; }, + /** + * The browser stopping the element of its own accord — a backgrounded tab pausing a video + * it considers silent. Distinct from pause(): nothing of ours asked for it, and there is no + * in-flight play() to interrupt, so the element simply stops. + */ + browserPause() { + video.paused = true; + }, + /** + * The mirror of `browserPause`: an element the browser hands back running, of its own accord + * and at whatever position it stopped it. Distinct from `settlePlay`, which resolves a + * `play()` we asked for — nobody asked for this one. + */ + browserResume() { + video.paused = false; + }, + }; + return video as unknown as HTMLVideoElement & { + plays: number; + settlePlay: () => void; + rejectPlay: () => void; + browserPause: () => void; + browserResume: () => void; }; - return video as unknown as HTMLVideoElement & { settlePlay: () => void; rejectPlay: () => void }; } describe('useDebatePlayback — an interrupted resume must not report failure (GEO-2895)', () => { @@ -214,6 +243,100 @@ describe('useDebatePlayback — an interrupted resume must not report failure (G expect(result.current.error).toBeNull(); }); + /** + * The generation check suppresses state writes, but it runs after the helper returns — and by + * then the helper's muted retry has already called `play()` on both elements. A viewer who + * paused mid-confirm got the videos back a moment later, muted, under a UI showing paused + * (GEO-2947). Cancellation has to reach inside the helper, not just guard what comes after it. + */ + it('does not restart the videos when the viewer grabs the scrubber mid-resume', async () => { + const { result, slot1, slot2 } = await mounted(); + // Unmuted, so the helper would otherwise take its force-mute-and-retry path. + slot1.muted = false; + slot2.muted = false; + + await act(async () => { + void result.current.resumeBoth(); + await Promise.resolve(); + result.current.beginScrub(); // the viewer grabs the scrubber, mid-confirm + await new Promise(resolve => setTimeout(resolve, 400)); + }); + + // One `play()` each — the attempt that was already under way. The muted retry must not have + // asked for a second, because nothing after it can take that back. + expect(slot1.plays).toBe(1); + expect(slot2.plays).toBe(1); + expect(slot1.paused).toBe(true); + expect(slot2.paused).toBe(true); + expect(result.current.error).toBeNull(); + }); + + /** Same for a card scrolled off screen mid-confirm — `suspend` takes ownership too. */ + it('does not restart the videos when the card is scrolled away mid-resume', async () => { + const { result, slot1, slot2 } = await mounted(); + slot1.muted = false; + slot2.muted = false; + + await act(async () => { + void result.current.resumeBoth(); + await Promise.resolve(); + result.current.suspend(); + await new Promise(resolve => setTimeout(resolve, 400)); + }); + + expect(slot1.plays).toBe(1); + expect(slot2.plays).toBe(1); + expect(slot1.paused).toBe(true); + expect(slot2.paused).toBe(true); + expect(result.current.playing).toBe(false); + }); + + /** + * The drift correction has the same reason to stand down mid-resume as the split-pair check + * did, and for a while it did not. A resume is a split pair by construction — slot 2, the + * cue-less WebM, is routinely the later of the two to start — so a tick landing in the confirm + * window sees a gap that is not drift and answers it by nudging (or hard-seeking) the element + * that is still trying to begin, which on these files means a parse walk competing with the + * start it is meant to help (GEO-2828). + */ + it('does not correct drift against a video that is still starting', async () => { + const { result, slot1, slot2 } = await mounted(); + + await act(async () => { + void result.current.resumeBoth(); + await Promise.resolve(); + // Slot 1 is up and has played on; slot 2 is still parsing, so it reads far "behind". + slot1.settlePlay(); + slot1.currentTime = 10; + slot2.currentTime = 0; + result.current.onPlaybackTick(); + await Promise.resolve(); + }); + + // Left alone: not dragged to slot 1's position, and not put on a corrective rate. + expect(slot2.currentTime).toBe(0); + expect(slot2.playbackRate).toBe(1); + }); + + /** The control: once the resume has settled, ordinary drift is corrected as before. */ + it('still corrects drift once the resume has settled', async () => { + const { result, slot1, slot2 } = await mounted(); + + await act(async () => { + void result.current.resumeBoth(); + await Promise.resolve(); + slot1.settlePlay(); + slot2.settlePlay(); + await new Promise(resolve => setTimeout(resolve, 50)); + }); + + slot1.currentTime = 10; + slot2.currentTime = 9.5; // beyond the nudge threshold, inside the seek one + act(() => result.current.onPlaybackTick()); + + expect(slot2.playbackRate).not.toBe(1); + }); + /** And the guard must not swallow a real block — the autoplay-policy error still surfaces. */ it('still surfaces a genuine failure to start', async () => { const { result, slot1, slot2 } = await mounted(); @@ -258,6 +381,14 @@ describe('useDebatePlayback — an interrupted resume must not report failure (G }); }); +/** + * GEO-2947. A debate the viewer is listening to has to survive them clicking into another + * window. Nothing in the player pauses on blur or on hide — but a backgrounded tab is where a + * browser stops a silent