Skip to content

fix(debates): videos stuck on Loading…, plus player control updates #2449

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Merged
ohohoreilly merged 8 commits into from
Sep 20, 2026
Merged

fix(debates): videos stuck on Loading…, plus player control updates #2449

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
8 commits
Select commit Hold shift + click to select a range
884371b
fix(debates): stop debate videos sitting on "Loading…" forever
o-p-o-p-o Sep 17, 2026
024883c
feat(debates): persistent play/pause beside mute, click feedback, lar…
o-p-o-p-o Sep 17, 2026
bdd6af0
feat(debates): use the explore claims icon in the fullscreen interact…
o-p-o-p-o Sep 17, 2026
d77b045
Potential fix for pull request finding
o-p-o-p-o Sep 17, 2026
df81e5b
Potential fix for pull request finding
o-p-o-p-o Sep 17, 2026
d78a0e0
Merge remote-tracking branch 'origin/master' into debate-video-updates
o-p-o-p-o Sep 18, 2026
b2869d6
Merge master into debate-video-updates
ohohoreilly Sep 20, 2026
0361977
Merge branch 'master' into debate-video-updates
ohohoreilly Sep 20, 2026
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
102 changes: 75 additions & 27 deletions apps/web/core/debates/browse/debate-feed-player.tsx
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -18,7 +18,7 @@ import { Avatar } from '~/design-system/avatar';
import { RetrySmall } from '~/design-system/icons/retry-small';
import { Text } from '~/design-system/text';

import { Play, Speaker, SpeakerMuted } from './icons';
import { Pause, Play, Speaker, SpeakerMuted } from './icons';
import { WinnerVoteButton } from './winner-vote-button';

type DebateFeedPlayerProps = {
Expand Down Expand Up @@ -100,6 +100,25 @@ export function DebateFeedPlayer({ debate, active, preload = false, votes }: Deb
const showReplay = ready && playbackEnded && !hasVoted;
const showPausedGlyph = ready && userPaused && !playbackEnded;

// Clicking the video briefly flashes the action it just took — feedback only, not a control.
const [flash, setFlash] = React.useState<{ icon: 'play' | 'pause'; visible: boolean }>({
icon: 'play',
visible: false,
});
const flashTimeoutRef = React.useRef<ReturnType<typeof setTimeout> | null>(null);
React.useEffect(
() => () => {
if (flashTimeoutRef.current) clearTimeout(flashTimeoutRef.current);
},
[]
);
const toggleFromVideo = () => {

@ohohoreilly ohohoreilly Sep 18, 2026

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The click flash fires on the "Loading…" placeholder.

toggleFromVideo is not gated on ready, so clicking the placeholder on desktop flashes a play glyph even though no <video> exists yet and resumeBoth returns immediately.

That is feedback for an action that did not happen, shown on exactly the state this PR is fixing. Gating setFlash on ready is enough.

setFlash({ icon: playing ? 'pause' : 'play', visible: true });
if (flashTimeoutRef.current) clearTimeout(flashTimeoutRef.current);
flashTimeoutRef.current = setTimeout(() => setFlash(current => ({ ...current, visible: false })), 600);
togglePlayback();
};

return (
<div ref={measurement.elementRef} className="group relative flex flex-col gap-2">
<DebaterVideo
Expand All @@ -112,31 +131,45 @@ export function DebateFeedPlayer({ debate, active, preload = false, votes }: Deb
mutedByUser={mutedByUser}
isResuming={isResuming}
onPlaybackTick={onPlaybackTick}
onToggle={togglePlayback}
onToggle={toggleFromVideo}
votes={votes}
topLeft={
showReplay ? (
<ControlCircle ariaLabel="Replay debate" onClick={playFromStart}>
<RetrySmall />
</ControlCircle>
) : ready ? (
// Feed debates autoplay muted, so the unmute control stays visible during
// playback — otherwise there's no way to hear audio. Once unmuted it recedes
// to hover-only.
<ControlCircle
ariaLabel={mutedByUser ? 'Unmute' : 'Mute'}
onClick={() => {
measurement.control(mutedByUser ? 'unmute' : 'mute');
setMutedByUser(current => !current);
}}
className={
mutedByUser
? undefined
: 'opacity-0 transition-opacity group-hover:opacity-100 focus-visible:opacity-100'
}
>
{mutedByUser ? <SpeakerMuted /> : <Speaker />}
</ControlCircle>
ready ? (
<div className="flex items-center gap-2">
{/* Desktop: a persistent play/pause beside the mute control. Mobile keeps the
centred paused glyph and tap-to-toggle instead. */}
<ControlCircle
ariaLabel={playing ? 'Pause debate' : playbackEnded ? 'Replay debate' : 'Play debate'}

@ohohoreilly ohohoreilly Sep 18, 2026

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Two adjacent buttons with the same accessible name.

On desktop at the end of an unvoted debate, showReplay is true, so the second control is the RetrySmall "Replay debate" button — and this label resolves to "Replay debate" too. Both call through to playFromStart.

A screen reader announces "Replay debate button" twice with nothing to distinguish them. The PR body flags the visual overlap here; the duplicated accessible name is the sharper half of the same problem.

onClick={togglePlayback}
className="md:hidden"
>
{playing ? <Pause /> : <Play />}
</ControlCircle>
{showReplay ? (
<ControlCircle ariaLabel="Replay debate" onClick={playFromStart}>
<RetrySmall />
</ControlCircle>
) : (
// Feed debates autoplay muted, so the unmute control stays visible during
// playback — otherwise there's no way to hear audio. Once unmuted it recedes
// to hover-only on desktop; touch has no hover, so on mobile it stays visible
// or there'd be no way to find it again.
<ControlCircle
ariaLabel={mutedByUser ? 'Unmute' : 'Mute'}
onClick={() => {
measurement.control(mutedByUser ? 'unmute' : 'mute');
setMutedByUser(current => !current);
}}
className={
mutedByUser
? undefined
: 'opacity-0 transition-opacity group-hover:opacity-100 focus-visible:opacity-100 md:opacity-100'
}
>
{mutedByUser ? <SpeakerMuted /> : <Speaker />}
</ControlCircle>
)}
</div>
) : null
}
/>
Expand All @@ -150,7 +183,7 @@ export function DebateFeedPlayer({ debate, active, preload = false, votes }: Deb
mutedByUser={mutedByUser}
isResuming={isResuming}
onPlaybackTick={onPlaybackTick}
onToggle={togglePlayback}
onToggle={toggleFromVideo}
votes={votes}
scrubber={
ready ? (
Expand All @@ -176,17 +209,29 @@ export function DebateFeedPlayer({ debate, active, preload = false, votes }: Deb
}
/>

{/* Mobile only — desktop has the persistent play/pause beside the mute control. */}
{showPausedGlyph && (
<button
type="button"
aria-label="Resume debate"
onClick={togglePlayback}
className="absolute top-1/2 left-1/2 z-20 grid size-16 -translate-x-1/2 -translate-y-1/2 place-items-center rounded-full bg-white text-text shadow-card"
className="absolute top-1/2 left-1/2 z-20 hidden size-16 -translate-x-1/2 -translate-y-1/2 place-items-center rounded-full bg-white text-text shadow-card md:grid"
>
<Play />
</button>
)}

{/* Desktop only — mobile already shows the centred paused glyph in this spot. */}
<div
aria-hidden
className={cx(
'pointer-events-none absolute top-1/2 left-1/2 z-20 grid size-16 -translate-x-1/2 -translate-y-1/2 place-items-center rounded-full bg-white text-text shadow-card transition-[opacity,scale] duration-300 md:hidden',
flash.visible ? 'scale-100 opacity-100' : 'scale-110 opacity-0'
)}
>
{flash.icon === 'pause' ? <Pause /> : <Play />}
</div>

{error && (
<Text as="p" variant="metadata" color="red-01" className="absolute inset-x-0 -bottom-6 text-center">
{error}
Expand Down Expand Up @@ -386,7 +431,10 @@ function ControlCircle({
event.stopPropagation();
onClick();
}}
className={cx('grid size-8 place-items-center rounded-full bg-white text-text shadow-light', className)}
className={cx(
'grid size-10.5 place-items-center rounded-full bg-white text-text shadow-light [&>svg]:scale-[1.3]',
className
)}
>
{children}
</button>
Expand Down
35 changes: 35 additions & 0 deletions apps/web/core/debates/use-debate-playback.test.tsx
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,5 +1,7 @@
import { act, renderHook, waitFor } from '@testing-library/react';

import * as React from 'react';

import { afterEach, beforeEach, describe, expect, it, vi } from 'vitest';

import type { Debate, DebateMediaTurnSegment } from './api';
Expand Down Expand Up @@ -106,6 +108,39 @@ describe('useDebatePlayback — playback URLs survive re-activation (GEO-2895)',

await waitFor(() => expect(result.current.urls.slot1).not.toBeNull());
});

// StrictMode (on by default in dev) mounts, cleans up and re-runs every effect before the
// first fetch settles. The re-run must not be skipped as "already fetched", or the cancelled
// first run drops the URLs and the card sits on "Loading…" forever.
it('loads URLs under StrictMode', async () => {
const { result } = renderHook(() => useDebatePlayback(debateFixture(), true), { wrapper: React.StrictMode });

await waitFor(() => expect(result.current.urls.slot1).not.toBeNull());
expect(result.current.urls.slot2).not.toBeNull();
});

it('loads URLs when the card is scrolled away and back before the first fetch settles', async () => {
let settle: () => void = () => {};
const gate = new Promise<void>(resolve => {
settle = resolve;
});
mocks.recordingUrl.mockImplementation(async ({ filename }: { filename: string }) => {
await gate;
return { url: `https://cdn.test/${filename}?sig=abc` };
});

const debate = debateFixture();
const { result, rerender } = renderHook(({ active }) => useDebatePlayback(debate, active), {
initialProps: { active: true },
});

rerender({ active: false });
rerender({ active: true });
await act(async () => settle());

await waitFor(() => expect(result.current.urls.slot1).not.toBeNull());
expect(result.current.urls.slot2).not.toBeNull();
});
});

/**
Expand Down
23 changes: 11 additions & 12 deletions apps/web/core/debates/use-debate-playback.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -271,6 +271,11 @@ export function useDebatePlayback(debate: Debate, enabled: boolean) {
//
// `useRecordingUrl` is a mutation rather than a query, so nothing upstream caches this —
// every discarded URL is a real round trip.
//
// The key is claimed only once URLs are committed, never while a request is in flight. A
// cleanup that lands mid-flight (StrictMode's dev double-run, or scrolling away and back
// before the fetch settles) cancels that run, and a claim taken up front would make the
// re-run skip as "already fetched" — leaving the card on "Loading…" forever.
const fetchedForRef = React.useRef<string | null>(null);

React.useEffect(() => {
Expand All @@ -286,10 +291,7 @@ export function useDebatePlayback(debate: Debate, enabled: boolean) {
if (fetchedForRef.current === recordingsKey) return;

let cancelled = false;
fetchedForRef.current = recordingsKey;
const releaseKey = () => {
if (fetchedForRef.current === recordingsKey) fetchedForRef.current = null;
};
fetchedForRef.current = null;

@ohohoreilly ohohoreilly Sep 18, 2026

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Dropping the up-front claim also drops in-flight de-duplication.

The comment just above notes that the explore feed activates on a bare intersectionRatio >= 0.6 with no hysteresis, so a single drag can flip enabled several times. Each flip landing inside the first request's window now starts two more signed-URL POSTs, and useRecordingUrl is a mutation so nothing caches them — 2N round trips on a slow connection where it used to be 2.

Worth noting the amplification lands hardest exactly where the bug is worst: a slow connection widens the in-flight window that causes both.

Keeping a promise per recordingsKey and having later runs await it would fix the hang without the amplification.

setUrls({ slot1: null, slot2: null });
// A different debate's clocks start over; carrying this across would strand the new one
// at the old one's position.
Expand All @@ -301,17 +303,14 @@ export function useDebatePlayback(debate: Debate, enabled: boolean) {
getRecordingPlaybackUrlRef.current({ debateId: debate.id, filename: slot2RecordingFilename }),
])
.then(([slot1Result, slot2Result]) => {
// Scrolled away mid-flight: nothing is committed, so release the key or the card
// would hold a claim on URLs it never received and never fetch again.
if (cancelled) {
releaseKey();
return;
}
// Cancelled mid-flight: commit nothing and leave the key unclaimed so the next run
// fetches again.
if (cancelled) return;
fetchedForRef.current = recordingsKey;

@ohohoreilly ohohoreilly Sep 18, 2026

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The key is claimed before the commit, so a throw in between recreates the permanent "Loading…" this PR exists to remove.

The claim happens on the line above setUrls, and the .catch no longer releases it on the grounds that "the key was never claimed". That holds for a rejected request, but not for anything that throws after line 215 and before the commit lands.

Concretely: geoChatRequest returns undefined on a 204 (core/debates/api.ts:1642), so slot1Result.url throws a TypeError once the key is already claimed. The card shows an error, urls stays null, and every later re-activation hits the fetchedForRef.current === recordingsKey early return — so it sits on "Loading…" forever with no retry, which is precisely the failure class this PR is fixing.

The old unconditional releaseKey() in .catch covered this case.

Either move the claim below setUrls(...), or keep a release in the catch.

setUrls({ slot1: slot1Result.url, slot2: slot2Result.url });
})
.catch(caught => {
// Same on failure, otherwise one error leaves the card permanently on "Loading…".
releaseKey();
// The key was never claimed, so the next activation retries.
if (!cancelled) setError(caught instanceof Error ? caught.message : 'Could not load recordings.');
});

Expand Down
Loading