fix(debates): videos stuck on Loading…, plus player control updates - #2449
Conversation
useDebatePlayback claimed its "already fetched" key before the signed-URL request settled. When a cleanup landed mid-flight — StrictMode's dev double-run, or scrolling a card away and back before the fetch settled — the re-run skipped as already fetched while the cancelled run discarded its URLs, leaving the card on the placeholder. Claim the key only once URLs are committed. Re-activating a card that already holds URLs still does not refetch (GEO-2895). Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
…ger controls - Desktop: an always-visible play/pause control next to mute replaces the centred paused glyph. Clicking the video flashes a play/pause glyph briefly as feedback; it is not clickable. - Corner controls are 30% larger (32px -> 42px). - Mobile: the mute control no longer disappears after unmuting. It receded to hover-only, and touch has no hover. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
|
The latest updates on your projects. Learn more about Vercel for GitHub.
|
There was a problem hiding this comment.
🟡 Changes recommended
The new control styling may not scale icons correctly, and replay state exposes a misleading accessible label.
Get a fresh assessment by requesting another Copilot review.
Pull request overview
Fixes debate video loading races and updates feed-player controls.
Changes:
- Defers URL-fetch tracking until URLs are committed.
- Adds regression tests for StrictMode and interrupted activation.
- Adds responsive play/pause feedback and larger controls.
File summaries
| File | Description |
|---|---|
use-debate-playback.ts |
Prevents cancelled URL requests from permanently blocking retries. |
use-debate-playback.test.tsx |
Tests interrupted and StrictMode fetch behavior. |
debate-feed-player.tsx |
Updates playback controls, feedback, mute visibility, and sizing. |
Review details
- Files reviewed: 3/3 changed files
- Comments generated: 2
- Review effort level: Lite
💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.
…ion bar The fullscreen feed's Claims button (vertical rail on desktop, pill bar on mobile) drew InfoSmall, a question mark in a ring. Explore's debate card uses ExploreClaimsIcon. Use that in both places so the claims icon is the same everywhere. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Co-authored-by: Copilot Autofix powered by AI <175728472+Copilot@users.noreply.github.com>
Co-authored-by: Copilot Autofix powered by AI <175728472+Copilot@users.noreply.github.com>
# Conflicts: # apps/web/core/debates/use-debate-playback.test.tsx
ohohoreilly
left a comment
ohohoreilly
left a comment
There was a problem hiding this comment.
Reviewed at d78a0e0, 4 files / +124 -42. One medium, four low.
The fix itself is right, and it matters more than the PR title suggests. Not claiming fetchedForRef until the URLs commit is the correct shape, and both new tests genuinely fail against master's hook, so they are real regression tests rather than restatements. Worth knowing: this appears to be the cause of a live user report tracked on GEO-2950 — grey panels that never resolve, reported from a 28-space Explore feed where cards activate and deactivate constantly. "Scrolling away and back before the fetch settles" fits that exactly. It would be worth confirming with the reporter once this lands.
It does not subsume GEO-2965, which is the asymmetric case — both panels holding URLs but painting at different times, downstream of this fetch. Neither should be closed on the strength of the other.
On the overlap with #2453
Both PRs touch use-debate-playback.ts and debate-feed-player.tsx, and I flagged a likely conflict earlier. Having actually run the merge of the two heads rather than comparing hunk ranges: only use-debate-playback.test.tsx conflicts. Both source files auto-merge cleanly, with isResuming threading and lastRunningPlayheadRef.current = null surviving alongside this PR's changes. So the collision is real but much smaller than I first said, and it is confined to the test file.
Verified, and deliberately not raised as findings
- The
md:breakpoints are correct. This repo redefinesmdas@media (max-width: 767px)(styles/styles.css:277), somd:hiddenis desktop-only andhidden md:gridis mobile-only — matching the stated intent rather than inverting it. size-10.5andscale-[1.3]compile under Tailwind v4.3.2 (2.625remandscale: 1.3).- All 13
use-debate-playbacktests and all 194core/debates/browsepluspartials/exploretests pass on the PR head.
| // Cancelled mid-flight: commit nothing and leave the key unclaimed so the next run | ||
| // fetches again. | ||
| if (cancelled) return; | ||
| fetchedForRef.current = recordingsKey; |
There was a problem hiding this comment.
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.
| const releaseKey = () => { | ||
| if (fetchedForRef.current === recordingsKey) fetchedForRef.current = null; | ||
| }; | ||
| fetchedForRef.current = null; |
There was a problem hiding this comment.
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.
| {/* 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'} |
There was a problem hiding this comment.
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.
| /> | ||
| <CircleAction label={String(commentCount)} onClick={onComment} icon={<Comment />} ariaLabel="Comments" /> | ||
| <CircleAction label={String(claimsCount)} onClick={onClaims} icon={<InfoSmall />} ariaLabel="Claims" /> | ||
| <CircleAction label={String(claimsCount)} onClick={onClaims} icon={<ExploreClaimsIcon />} ariaLabel="Claims" /> |
There was a problem hiding this comment.
Claims renders 25% smaller than the icons beside it.
ExploreClaimsIcon hard-codes width="12" height="12", while Comment and Share next to it are 16x16 — in both the vertical rail and the pill bar (also at :68).
Since the stated purpose of the change is icon consistency, this is worth a sizing className, which the component already accepts.
| }, | ||
| [] | ||
| ); | ||
| const toggleFromVideo = () => { |
There was a problem hiding this comment.
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.
|
Follow-up to my review: #2453 has now merged, so this needs a rebase before it can land. I ran the merge rather than guessing at it. The damage is small and confined:
So it is one test file to resolve, not the two source files. Worth doing soon: GEO-2895 is sitting in review waiting on this PR, and the fix here looks like the cause of a live user report on GEO-2950. |
Resolves two conflicts left by 13 commits of master, including #2453 (GEO-2947). `debate-interaction-bar.tsx` takes master's side. #2462 ("unify claim action icon") already landed the same intent this branch was reaching for, and deleted `partials/explore/explore-claims-icon` in the process -- so keeping this branch's version would mean restoring a component master deliberately removed. The Claims icon is now `Warning`, matching its neighbours. Git merged around the deletion silently; the tests are what caught it. `use-debate-playback.test.tsx` takes the union of both import lists: master added `afterEach`, this branch added React, and both are used. Merged rather than rebased so nothing on this branch is rewritten. Verified: 137 tests across 9 files in core/debates pass on the result.
|
I merged Two conflicts, and one of them is a decision you should ratify rather than just inherit.
Worth knowing how that surfaced, because it is a trap: git merged around the deletion silently. A removed file and an unchanged import do not textually conflict, so the merge looked clean and the tests were what caught it — That also retires one of my earlier review findings here: the note about Verified on the merged result: 137 tests across 9 files in Two things still pending on this PR:
This fix looks like the cause of a live user report on GEO-2950, and GEO-2895 is sitting in review waiting on it, so it is worth getting over the line. |
#2449 rewrote the player's control cluster, which this PR had also been editing. Resolution: - Took #2449's cluster wholesale. Both sides had independently added a persistent play/pause beside mute; theirs is the designed one, with the mobile centred glyph, the click-feedback flash and the larger controls. - Kept `no-hover:opacity-100` on their mute rule alongside their `md:opacity-100`. The two ask different questions: a tablet in landscape is wider than `md` and still has no hover, so width alone leaves the control faded but tappable there — the bug this PR fixed. - Kept this PR's claim props on both tiles, the seam subtitle, the extracted `useOpenDebaterProfile`, and the shared playback-end epsilon; kept master's URL-fetch key fix and `releaseVideo` effect. - The auto-merge dropped master's `showReplay`/`showPausedGlyph` and re-introduced a `votes` prop and `WinnerVoteButton` that this PR's tile redesign had removed. Both caught by diffing the merged file against master's and accounting for every line. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
…fore (GEO-2950) #2449 removed a permanent "Loading…" by claiming `fetchedForRef` only once URLs are committed, rather than up front. The claim still happens one line too early, which leaves a narrower version of the same bug. `geoChatRequest` returns `undefined` on a 204 (`core/debates/api.ts`), so `slot1Result.url` is a TypeError rather than a rejected request. It throws *after* the key is claimed and lands in the `.catch`, which deliberately does not release — so the key stands over null URLs, the card shows the error, and every later activation takes the `fetchedForRef.current === recordingsKey` early return and never asks again. The card is then stuck behind "Loading…" for the rest of the session, which is exactly what #2449 set out to remove. Reading both fields before claiming makes "the key was never claimed" true for every throw that can reach the catch. Deliberately NOT fixed by releasing in the `.catch`. That is what the code did before #2449, and because the key is identical across attempts it let a stale cancelled attempt free a claim a newer in-flight one owned — the original defect. Ordering is the fix; the catch stays as it is, with a comment saying why. The regression test drives a 204-shaped answer and asserts the card asks again after scrolling away and back. Checked that it bites: it fails against the previous ordering. Verified: 105 files / 1612 tests pass across core/debates; tsc clean.
…fore (GEO-2950) (#2476) #2449 removed a permanent "Loading…" by claiming `fetchedForRef` only once URLs are committed, rather than up front. The claim still happens one line too early, which leaves a narrower version of the same bug. `geoChatRequest` returns `undefined` on a 204 (`core/debates/api.ts`), so `slot1Result.url` is a TypeError rather than a rejected request. It throws *after* the key is claimed and lands in the `.catch`, which deliberately does not release — so the key stands over null URLs, the card shows the error, and every later activation takes the `fetchedForRef.current === recordingsKey` early return and never asks again. The card is then stuck behind "Loading…" for the rest of the session, which is exactly what #2449 set out to remove. Reading both fields before claiming makes "the key was never claimed" true for every throw that can reach the catch. Deliberately NOT fixed by releasing in the `.catch`. That is what the code did before #2449, and because the key is identical across attempts it let a stale cancelled attempt free a claim a newer in-flight one owned — the original defect. Ordering is the fix; the catch stays as it is, with a comment saying why. The regression test drives a 204-shaped answer and asserts the card asks again after scrolling away and back. Checked that it bites: it fails against the previous ordering. Verified: 105 files / 1612 tests pass across core/debates; tsc clean.
Three changes to the fullscreen debate feed.
1. Debate videos sat on "Loading…" forever
Both video panels on a debate page never left the placeholder, even though every request to the debates API returned 200.
Cause.
useDebatePlaybackmarked a debate's recordings as "already fetched" as soon as it started the signed-URL request. If the effect was cleaned up before that request settled, the re-run saw the mark and skipped, while the cancelled run threw its URLs away. Nothing ran again.Two ways to hit it:
It came in with the GEO-2895 re-activation guard (#2429, #2431).
Fix. The mark is set only once URLs are committed. A cancelled run leaves nothing behind, so the next run fetches. The GEO-2895 behaviour holds: re-activating a card that already has its URLs does not refetch or blank it.
Tests. Two new cases in
use-debate-playback.test.tsx, both failing before the fix:React.StrictMode2. Player controls
pointer-events-none,aria-hiddenMobile otherwise keeps its current behaviour: no corner play button, and the centre glyph while paused.
The larger size applies to every corner control, including replay and mobile mute, since they share
ControlCircle.One overlap to be aware of: when a debate ends unvoted, the mute spot becomes Replay, and the new play button next to it also replays. Easy to hide the play button in that state if we'd rather not have both.
3. Claims icon matches Explore
The fullscreen feed's Claims button drew
InfoSmall, a question mark in a ring. Explore's debate card usesExploreClaimsIcon, a thin ring with an "i". The interaction bar now usesExploreClaimsIconin both its layouts (the desktop vertical rail and the mobile pill bar), so the claims icon is the same everywhere. No new icon: it reuses Explore's component.Testing
vitestovercore/debatesandpartials/explore: all pass (1,631 tests before the last control tweaks; the 194 feed/explore tests re-run after them).🤖 Generated with Claude Code