fix(debates): keep playback running when the tab is backgrounded (GEO-2947) - #2453
Conversation
…-2947) Switching to another window took a debate silent, and it stayed silent until the card was clicked. Nothing in the player listens for blur or visibilitychange, so the pause was not a decision we made — it was the sync step reacting to one the browser made for us. A backgrounded tab is where a browser stops a <video> it considers silent. With two elements and only the speaking one audible, that stops the listening debater's element and leaves the speaker's running. The sync step read that split as "the browser stopped playback on us", paused the half that was still playing, and recorded it as a *user* pause — which is why auto-resume stood down and only a click brought it back. Dropping `playing` also dropped `audible`, so both elements re-muted on the way out. Off screen, the pair is now left alone: neither the drift correction nor the play/pause reconciliation runs while the document is hidden, so the audio the viewer is listening to keeps going. On the way back, a pair the browser did stop is restarted through `resumeBoth`, which re-seeks slot 2 to slot 1 first — so it resumes in step from where playback actually got to, with no reset of the playhead and no change to the mute preference. Visibility, not focus: a tab sitting on screen beside the window being typed in never had a reason to stop. Scrolling a card out of the viewport is a separate condition and still pauses, unchanged.
|
The latest updates on your projects. Learn more about Vercel for GitHub.
|
…back fix Both were the same mistake as the original bug, relocated. The split-pair guard stood down while hidden, but the tab is visible again by the time the return path resumes — and a resume *is* a split pair for as long as it takes to confirm, with slot 2 (a cue-less WebM) routinely the later of the two. Slot 1 emits timeupdate about four times a second throughout, so a tick landed in that window as a matter of course and paused the video that had just started, sticky. A resume in flight now counts as "not settled", alongside a hidden tab, and both corrections read the one predicate. The turn was still nulled when slot 1 was found paused. Off screen that is usually the browser stopping slot 1 — which is the muted element for all of slot 2's turn — and a null turn drops `audible`, which re-mutes slot 2 while it is still playing. The same silence, through the back door. Whose turn it is now comes off the playhead unless the pair is settled. Per-turn audio moves from the `muted` flag to `volume`. It stops the listening debater's element looking like a silent video to a backgrounded tab, and it leaves `muted` meaning only the viewer's own mute — which is already the only thing `playFromStart` and the autoplay fallback write it for, so the element no longer carries two notions of "muted" written from two places. Also recorded, rather than fixed: a pause made from outside the page (a media key) while the tab is hidden is indistinguishable from the browser's own, so it is resumed too.
Review pass — two confirmed bugs found and fixed (80f827a)Both were the original bug relocated rather than removed, so the first version only half-worked. 1. The return path re-created the bug it fixes. The split-pair guard stood down while hidden — but by the time Fixed by counting resumes in flight and folding it into one predicate with the visibility check: 2. The turn was still nulled by a browser-paused slot 1. The Also changed: per-turn audio moves from
|
There was a problem hiding this comment.
🟡 Changes recommended
A browser-paused element can cause silence at the next turn and rewind playback when the tab returns.
Get a fresh assessment by requesting another Copilot review.
Pull request overview
Updates debate playback to better survive browser backgrounding.
Changes:
- Defers synchronization while hidden and reconciles on return.
- Gates speaker audio using volume rather than
muted. - Adds background-playback and audio-gating tests.
File summaries
| File | Description |
|---|---|
use-debate-playback.ts |
Adds hidden-tab playback reconciliation. |
use-debate-playback.test.tsx |
Tests background playback behavior. |
debate-feed-player.tsx |
Moves per-turn audio gating to volume. |
debate-feed-player.test.tsx |
Tests volume and mute behavior. |
Review details
Suppressed comments (1)
apps/web/core/debates/use-debate-playback.ts:546
resume()always uses slot 1 as the clock and immediately seeks slot 2 to slot 1. If slot 1 was the silent element that the browser paused while slot 2 kept playing, returning to the tab rewinds slot 2 to slot 1's stale position and repeats everything heard in the background. Reconcile from the furthest debate-time reached by either element (accounting for recording offsets), or letresumeBothaccept the element/playhead that remained active; add a return-path case to the existing slot-1-paused regression.
// Both still running — the tab was backgrounded and the browser let it be. Nothing to do,
// and calling resumeBoth here would seek a pair that is already in step.
if (!primaryVideo.paused && !secondaryVideo.paused) return;
void resume();
- Files reviewed: 4/4 changed files
- Comments generated: 1
- Review effort level: Balanced
💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.
Both of Copilot's findings are the same assumption: slot 1 is the clock. It holds while the pair is in lockstep, and stops holding the moment a hidden tab stops one of them. If slot 1 is the element the browser stopped, its clock freezes where it stopped while slot 2 carries the debate on. Reading the playhead off it was wrong in two places, so `pairPlayheadSeconds` now answers it once for both: whichever element is actually running is the clock, slot 1 first, and slot 1 again when both are paused — the ordinary paused, scrubbing and pre-resume states are unchanged. - `updateTurnState` froze the turn, so audio never moved to the next speaker even when the running element had long since passed the boundary. - `resumeBoth` seeks the pair to the clock's position, so returning to the tab rewound slot 2 to slot 1's frozen position and replayed everything heard in the background — against the ticket's "returning to the tab: no reset". Standing down off screen also turns out to be sufficient only until the turn changes: at the boundary `audible` moves to the other recording, and if that is the stopped one the debate goes silent for the rest of it. The hidden path now aligns and starts that element. Narrow on purpose — it needs a genuinely split pair, so a tab where the browser stopped both (the muted feed default, which nobody is listening to) is left alone; a floor between attempts keeps a browser that re-stops it from costing a seek per tick; and a refused start records neither a pause nor an error, because refusing an off-screen start is the browser's prerogative.
Copilot review — both comments addressed (6a07430)Including the suppressed one, which was the more serious of the two. Replied inline on the visible comment; this covers the suppressed one and the audit it prompted. Suppressed comment (
|
| Site | Verdict |
|---|---|
updateTurnState playhead |
Was broken — froze the turn, so audio never reached the next speaker. Fixed. |
resumeBoth realignment |
Was broken — the rewind Copilot described. Fixed. |
| Drift correction dragging slot 2 to slot 1 | Already safe: guarded by !primaryVideo.paused, !primaryStalled and now pairIsSettled, so it can't drag slot 2 back to a frozen slot 1 (that guard is GEO-2828's). |
seekVideosTo |
Safe by construction — writes both elements from a debate-timeline playhead. |
primaryProgressRef stall detection |
Slot-1 only, but unreachable while hidden (pairIsSettled is false). |
playbackEnded / activeSlot / subtitle |
All derived from playheadSeconds, so fixed by the same change. |
playFromStart |
Seeks to 0 deliberately. Unaffected. |
The two broken sites now share one answer: pairPlayheadSeconds in playback-utils.ts, beside the other pure playback helpers, with its own unit tests. Whichever element is actually running is the clock, slot 1 first, and slot 1 again when both are paused — so the ordinary paused, scrubbing and pre-resume states are byte-for-byte unchanged.
Also fixed: the turn boundary (the visible comment)
The inline comment's scenario needed the playhead fix and a second change — details in the inline reply. Short version: when the turn moves to an element the browser stopped off screen, align it and start it; narrow enough that a tab where the browser stopped both is left alone, floored so a browser that re-stops it can't cost a seek per tick, and silent on refusal.
Verification
5 new tests (2 for the turn boundary and its negative control, 1 for the resume position, plus 4 unit tests on the new helper). The three new regression cases all fail against the previous commit and pass on this one.
core/debates + partials/explore: 114 files / 1646 tests green. tsc --noEmit reports only master's three. eslint clean.
The manual browser check in the PR description still stands — this all sharpens the handling of a browser-initiated pause, which remains inferred rather than observed.
There was a problem hiding this comment.
🟡 Changes recommended
Fully paused pairs can lose the later playhead and rewind playback when the tab returns.
Get a fresh assessment by requesting another Copilot review.
Review details
- Files reviewed: 6/6 changed files
- Comments generated: 1
- Review effort level: Balanced
A pair the browser stops off screen is not stopped at one instant. Slot 1 goes first, slot 2 plays on, and when slot 2 stops too every clock on the page is behind where the debate actually reached — slot 1 by however long slot 2 kept going. Reading the position off either element then rewinds playback over ground already heard, and `ended` (which reads as paused) arrives the same way. Reading the running element's clock therefore only solved half of it: it answers "which clock" but there is no clock left to ask once both have stopped. `pairPlayhead` now takes the caller's record of the last position observed while something was running, and prefers it to a frozen clock behind it. It reports whether the answer came off a live element, so the caller can keep that record current without re-deriving "is anything running" for itself. The record is only safe because it is dropped whenever the position is redefined from outside: every deliberate seek sets it (so a scrub backwards stays where the viewer put it rather than being dragged forward to the furthest point played), and changing debates clears it (so a new debate does not start stranded at the old one's position).
Second Copilot review — addressed (accf804)One inline comment this round, on the helper I added last round; no suppressed comments in this review ( The finding — confirmed
Correct, and my previous fix only solved half the problem: it answered which clock to read, but once both elements have stopped there is no clock left to ask. Not a corner case either — the pair isn't stopped at one instant. Slot 1 goes first (it's the silent element for all of slot 2's turn), slot 2 plays on, and when slot 2 stops too slot 1 is behind by however long that was. The class of error, enumeratedThe first review's finding was "slot 1 is not always the clock". This one is the deeper version: the pair's position cannot always be reconstructed from the elements at all — after a background stop, every clock on the page can be behind where the debate actually reached. Every site that derives position or progress:
Found by the audit, not reportedA remembered position is only safe if it's dropped whenever the position is redefined from outside. Two resets, and the second was not in the comment:
A note on the test, worth recordingMy first attempt at Copilot's sequence passed against the broken code. At 25s it's still slot 1's turn, so the turn-boundary restart from the previous commit had already moved slot 1's clock forward and masked the staleness. It only isolates the bug at 40s, inside slot 2's turn, where nothing restarts slot 1 — there it fails on the previous commit ( Verification7 new tests (1 hook-level for the reported sequence, 1 for the scrub reset, 5 on the helper). The manual browser check in the PR description still stands. |
There was a problem hiding this comment.
🔵 Needs a closer look
Volume-based gating causes both debaters to be audible on iOS Safari, which cannot programmatically change media volume.
Review details
Suppressed comments (1)
Previously missed (1) — in code that hasn't changed since the last review.
apps/web/core/debates/browse/debate-feed-player.tsx:246
- iOS Safari keeps
HTMLMediaElement.volumeat 1 and does not support programmatic volume changes. Since this PR also un-mutes both elements, mobile Safari will play both debaters audibly at once after the viewer unmutes. Fall back to muting the non-speaking element when the assigned volume does not stick, and cover that unsupported-volume case.
- Files reviewed: 6/6 changed files
- Comments generated: 0 new
- Review effort level: Balanced
…wner iOS Safari treats `volume` as read-only: the write is accepted and ignored, and the property stays at 1. Gating the per-turn audio on volume while also no longer muting the listening element therefore put both debaters on air at once there, the moment the viewer un-muted. The assignment is now read back, and where it doesn't stick the listener is muted exactly as it used to be. Mobile is out of scope for background playback anyway — iOS stops inline <video> on backgrounding regardless — so the fallback costs it nothing it had. Auditing the rest of that class — a DOM write that doesn't take, or that React cannot see — turned up the same shape in `muted`, which had two owners. `playBothWithMutedFallback` muted both elements for its retry and left them muted when the retry also failed. React only writes a DOM property when its own previous value differs, so a mute left behind that way is invisible to it and outlives every later render that says otherwise: the pair plays silently under a UI still offering a "mute" control. It now restores what it found on the way out. The successful retry still keeps the mute, because the caller pairs that outcome with the state change that makes it the rendered truth. `playFromStart` then had no reason to write `muted` at all — repairing that leak was the only thing it did. Keeping it re-created the divergence from the other side, since the value it knows (`mutedByUser`) stops being the value the player renders once the per-turn gate falls back to muting. Removed: the render owns `muted` now, and nothing writes behind it.
Third Copilot review — the suppressed comment was a real mobile regression (53a0100)
The finding — confirmed
Exactly right. Worth noting: the speaking element can't detect this — it asks for volume 1 and reads back 1, indistinguishable from success. That's fine, and it's why the check is per-element rather than a one-shot platform probe: it's the listener whose write visibly fails, and it's the listener that needs silencing. Each element detects it the first turn it has something to lose. The class, enumerated — and a second instance foundThe class is a DOM write that doesn't take effect, or that React can't see. Auditing it turned up the same shape in
The second row is the one I'd point a reviewer at: it's pre-existing, but this PR made it reachable in a new way, and it's the reason Verification5 new tests: 3 rendering under a stubbed read-only
All three rounds of comments — 2 inline, 2 suppressed — are now addressed. The manual browser check in the PR description still stands, and iOS is now explicitly worth a look too: unmute a debate in the feed on an iPhone and confirm you hear one debater at a time. |
There was a problem hiding this comment.
🟡 Changes recommended
Playhead recovery can rewind playback, and asynchronous mute restoration can override newer user intent.
Get a fresh assessment by requesting another Copilot review.
Review details
Suppressed comments (2)
Previously missed (2) — in code that hasn't changed since the last review.
apps/web/core/debates/playback-utils.ts:141
- When both elements are paused, this fallback ignores slot 2's final
currentTime. If slot 1 stops, slot 2 advances, and slot 2 then pauses between throttledtimeupdateticks,lastRunningSecondsis stale; the ensuingpausecallback already sees slot 2 as paused, so it cannot refresh the memory. Returning visible therefore seeks backward to the last tick. Include slot 2's frozen timeline position once itscurrentTimeshows that recording has started (while still avoiding its positive offset before startup), and add a regression without the explicit final running tick used by the current test.
apps/web/core/debates/use-debate-playback.ts:585 - This description is now incorrect:
resumeBothno longer uses slot 1 unconditionally, andseekVideosTorealigns both elements frompairPlayhead(possibly using slot 2 or remembered progress). Keeping the old explanation obscures the rewind fix implemented above.
- Files reviewed: 6/6 changed files
- Comments generated: 1
- Review effort level: Balanced
The restore added last commit was itself the bug it was fixing. It put back a `muted` value captured before a ~300ms await, so a viewer who muted during the attempt — from the control that stays visible, or from another card sharing the preference — had that decision overwritten with the older one. React had already committed `muted=true` and so believes the DOM says true; a write it disagrees with is one it never repairs, and the pair plays audibly under a UI showing muted. There is no value the helper can restore: it does not know what the caller renders `muted` from, and anything it captures is out of date by the time it could write it. So it no longer tries. The repair belongs to whoever renders the prop, and `DebateFeedPlayer` now re-asserts it — not during the attempt, which depends on the mute it just made, but the moment `isResuming` falls, which is itself what re-runs the effect. Also: the playhead memory is refreshed on ticks, and `timeupdate` is throttled in a background tab, so an element that stops between ticks stops somewhere the memory never saw — and its own `pause` arrives when it already reads as paused. Both frozen clocks are now weighed alongside the memory and the furthest wins, with slot 2's counted only once its `currentTime` shows the recording has played, so an untouched slot 2 can't report its start offset as debate progress. And two comments that had outlived their code: the reconcile effect still described a resume that seeks slot 2 to slot 1, and `playFromStart` still credited the helper with a restore that no longer exists.
Fourth Copilot review — all three addressed (23d1ad3)1 inline (replied) and 2 suppressed. All three were worth fixing; the inline one caught a bug I introduced in the previous commit while fixing this same class. Inline — the restore was itself the bug it fixedLast commit I made The fix is architectural rather than another patch: whoever renders Worth noting the other suggested remedy — restore from the hook's latest Suppressed 1 — the playhead memory has gaps
Correct, and the throttling makes it routine rather than rare. Added the regression without the explicit final running tick, as asked ( Suppressed 2 — stale comment, and the sweep it promptedRight, and the sweep found a second one that had gone stale in the previous commit: That's the honest lesson of this round: on a change that's been reworked five times, prose goes stale faster than code, and it goes stale silently. Both fixed, and I've re-read every comment I've added across the PR against the code as it now stands. Verification6 new tests (3 on the mute repair, 2 on the helper's frozen-clock handling, 1 hook-level for the no-tick sequence). Each new behavioural test was checked against the previous commit:
Across four rounds: 3 inline comments and 4 suppressed, all addressed. The manual checks in the PR description stand — desktop Chrome/Safari for background playback, and iOS for one debater at a time. |
…ivation The background recovery published a new playhead but left `turnState` alone, and `turnState` is what `audible` reads. A pair the browser let run through a turn boundary while the tab was hidden therefore came back with the volume still on the debater who had stopped speaking — the reconcile returns early in that case, so nothing else corrected it until a later media tick happened to. The turn is now derived from the recovered position like every other consumer of it. And "every other consumer" is the point: four call sites were binding the same debate metadata to `turnStateForTime` by hand, which is how the recovery came to move one without the other. They share one `turnAt` now, so a playhead that moves takes the turn with it by construction rather than by everyone remembering.
Eighth Copilot review — addressed (05d6ff4), and a recommendation to stop hereOne inline comment (replied), no suppressed section. The finding — confirmedThe background recovery published a new playhead but left The class, enumerated — and the real defectLast round I listed the tick-maintained state the recovery had to publish and said They now share one Verification: 1 new test ( Why I think this should be the last review roundNot because the findings stopped being real — this one was real, and so were the other seven. Because of what the sequence shows:
That last point is decisive. The PR splits cleanly in two: Unconditionally correct, needs no theory — don't convert a browser-initiated pause into a sticky Speculative, ~250 lines — the volume-vs- If Chrome does pause the silent element, the second half is load-bearing and should stay. If it doesn't, it is inert and should be cut, not further refined. A single manual check decides which — unmute a debate in the feed, switch to another app for ~30 seconds, and watch whether audio survives the next turn boundary — and it is worth more than the next several review rounds put together. An iOS pass (one debater audible at a time) is the other one. So: I'd stop reviewing and go verify. |
There was a problem hiding this comment.
🔵 Needs a closer look
Browser-specific background media behavior remains manually unverified, and two hook dependency issues remain.
Review details
Suppressed comments (1)
apps/web/core/debates/use-debate-playback.ts:603
seekBothnow calls the reactiveturnAtcallback, but the dependency list still names only the callback’s current implementation details. This can become stale ifturnAtgains another dependency and also violates exhaustive-deps; listturnAtitself instead.
setTurnState(turnAt(nextTime));
- Files reviewed: 6/6 changed files
- Comments generated: 1
- Review effort level: Balanced
…from `updateTurnState` and `seekBoth` call `turnAt` but listed the debate metadata `turnAt` happens to close over. That is correct only by coincidence — `turnAt` changes identity exactly when those two values do — and the coincidence ends the moment `turnAt` gains a dependency, at which point both callbacks go stale without anything saying so. Centralising the derivation last commit is worth little if its callers still reach around it. Nothing here is enforced: this project's eslint config pulls `eslint-config-next/typescript` rather than the full preset, so `react-hooks/exhaustive-deps` is not enabled and a stale dependency list lints clean. Running the rule ad hoc over the changed files reports nothing further; the one finding it does make, on `transcriptSegments`, predates this branch and is an over-invalidation rather than a staleness bug. `seekVideosTo` named `offsets.slot1` and `offsets.slot2` where every other consumer names `offsets`; same object, one fewer way for a third field to be added without its dependency following.
Ninth Copilot review — addressed (42d537a)One inline comment and one suppressed comment, which are the same defect at two sites ( The finding — confirmedBoth callbacks call One correction to the comment, and why this slipped
It would not, here. This project's eslint config pulls The class, enumerated — mechanically this timeRather than eyeball the other 14 dependency arrays, I ran the rule ad hoc over both changed files with a throwaway config. After the fix it reports nothing in either. Its one remaining finding — Also tidied 114 files / 1673 tests green. Recommendation unchanged: stop hereThis round is a fair illustration of where the remaining value is. The finding was real and worth fixing, but it is a maintainability defect — it cannot produce a wrong result today — and the genuinely useful discovery was about the repo's tooling, not this change. Meanwhile the review's own headline still reads "browser-specific background media behavior remains manually unverified", which is the same thing I have been saying since the first commit and the reason five of the nine rounds have found bugs in machinery serving an unobserved premise. Four of nine found a bug in the immediately preceding round's fix. The diff is +1,400/−28 for a fix that started at 90 lines. The next action that changes anything is 30 seconds in a real Chrome window: unmute a debate in the feed, switch to another app, and see whether audio survives the next turn boundary. That answers whether ~250 lines of this PR are load-bearing or should be deleted. No further review round can. |
There was a problem hiding this comment.
🔵 Needs a closer look
Drift correction can still seek or alter a video while its resume attempt is unsettled.
Review details
Suppressed comments (1)
Previously missed (1) — in code that hasn't changed since the last review.
apps/web/core/debates/use-debate-playback.ts:401
- The new in-flight guard is not applied to drift correction: this branch still runs whenever the tab is visible, even while
resumeBothis confirming a split pair. If slot 1 starts while slot 2 is still parsing, ticks can nudge or hard-seek slot 2 during its pending start—the exact unsettled windowpairIsSettledis intended to leave alone, and a hard seek can further delay cue-less WebM startup. Gate this correction onpairIsSettledas well.
- Files reviewed: 6/6 changed files
- Comments generated: 0 new
- Review effort level: Balanced
The in-flight guard was supposed to cover both corrections in `updateTurnState`, and a commit message and PR comment said it did. Only the split-pair check ever got it; the drift correction still ran whenever the tab was visible. It matters for the same reason the other one did. 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. On these files a seek is a parse walk (GEO-2828), so the correction competes with the start it is meant to be helping. The guard was written but never applied: the edit matched the condition as it had been before Prettier wrapped it across lines, so it replaced nothing and said nothing. Worth naming, because the same silence is what let it survive three rounds of review of the surrounding code.
Tenth Copilot review — the suppressed comment was right, and I had claimed otherwise (65070f9)
I said this was already done. It was not.In the round-2 summary I wrote that the in-flight guard was folded into "one predicate ... read by both corrections rather than each growing its own condition". Only the split-pair check ever got it. The drift correction still ran whenever the tab was visible, exactly as the comment says. The cause is worth naming because it is not a thinking error. The edit that was supposed to apply the guard matched the condition as it looked before Prettier wrapped it across several lines. It matched nothing, changed nothing, and reported nothing — every other edit in that batch asserted its target was present first; that one did not. So the guard was written, described in a commit message, described again here, and never applied. Three subsequent reviews read the surrounding lines without anyone, me included, re-checking the claim against the file. The finding — confirmedA resume is a split pair by construction: slot 2, the cue-less WebM, is routinely the later of the two to start. A tick landing in the confirm window sees a gap that is not drift and answers it by nudging — or, once the seek floor has elapsed, hard-seeking — the element that is still trying to begin. On these files a seek is a parse walk (GEO-2828), so the correction directly slows the start it is competing with. The class, enumerated — checked against the file this timeThe defect class is not "drift correction"; it is a claimed change that was never made. So rather than re-reading my own comments, I grepped the file for every invariant this PR has claimed, one by one:
One gap, now closed. Everything else this PR has claimed is in the file. Verification2 new tests: 114 files / 1675 tests green. RecommendationUnchanged — stop the review rounds — but this round genuinely earns its keep, and for a different reason than the others: it caught a false claim, not just a bug. That is the one failure mode a summary comment cannot self-correct, and it argues for a human spot-check of the claims in this thread against the diff before merge rather than for more automated rounds. The blocking item is still the browser check. Ten rounds in, five of the findings have been in machinery serving a premise that has never been observed, and this one was in a guard protecting that machinery. |
GEO-2949 landed in the same two files and reached the same conclusion about one of them from the other end. It introduced `turnStateAt` — one memoized "whose turn is it at time T" the whole hook shares — which is exactly the centralisation added here last commit as `turnAt`, except that it prefers the boundaries the render actually cut over the format's allowance. So `turnAt` is deleted rather than merged: every call site here now reads `turnStateAt`, and the background recovery gets the better answer for free. Kept from this branch on the two conflicting hunks: the hidden-tab guards in `updateTurnState` (`pairIsSettled` on the drift and split-pair corrections, the turn left alone when a backgrounded tab stopped slot 1), the turn-boundary restart, and the `offsets` dependency in place of its two fields. The test file is reconstructed rather than patched: both sides appended describes at the same point, so it is master's file plus this branch's `fakeVideo` additions (a `play()` counter and `browserPause`), the four cancellation and drift tests inside the shared "interrupted resume" describe, and the GEO-2947 describe whole. Its `beforeEach` now clears `mocks.turnSegments` like every other, so these tests keep falling back to the allowance — they are about which element is running, not where the boundaries sit. 114 files / 1688 tests pass on the merge.
Merged master (07b9fc1) — conflicts resolvedGEO-2949 (#2454) landed in the same two files while this was open. Worth reading before the diff, because it changed the shape of one part of this branch rather than just colliding with it.
|
ohohoreilly
left a comment
ohohoreilly
left a comment
There was a problem hiding this comment.
Review of the diff at 07b9fc177, 6 files, +1482/-27. Five findings below, ordered by severity; the two mediums at the top are the ones I would settle before merge.
Also checked and found sound: playBothWithMutedFallback's isCancelled placement (the only window it leaves is covered by the caller's generation check), the resumesInFlightRef count/finally pairing including overlapping resumes, the muted repair landing on every outcome, the volumeIsWritable probe not looping, lastRunningPlayheadRef resetting on seekVideosTo and on a recordings change, the offsets dependency changes being memo-identity equivalent, the restructured end-of-timeline branches being behaviour-preserving in the settled case, and the claim that nothing else in apps/web pauses feed playback on blur/visibilitychange (use-playback-analytics.ts:116 only breaks the measurement clock).
I did not run the test suite.
| lastRunningSeconds: number | null = null, | ||
| trustSecondary = false | ||
| ): PairPlayhead { | ||
| if (primary && !primary.paused) return { seconds: primary.currentTime + offsets.slot1, live: true }; |
There was a problem hiding this comment.
The playhead ratchet is not monotone — a hidden tab can rewind the debate.
pairPlayhead returns primary.currentTime + offsets.slot1 as soon as !primary.paused, without folding in lastRunningSeconds or slot 2. The caller at use-debate-playback.ts:361 then assigns unconditionally:
if (position.live || hidden) lastRunningPlayheadRef.current = playhead;under a comment asserting that playhead already folds the previous record in, so this cannot go backwards. It can.
Tab hidden, slot 2 is the speaker. The browser stops slot 1 at debate-time 100 while slot 2 runs on to 130, so the ref reaches 130 through the trustSecondary live branch. Slot 1 then comes back un-paused at its frozen 100 — either the browser un-suspends it, or it is un-paused-but-stalled, which paused === false cannot distinguish and which this file already documents as slot 1's normal failure mode (GEO-2828, "a stalled slot 1 can leave it far ahead"). The next tick takes the first live branch and overwrites the ref 130 -> 100.
On return, recoverBackgroundPlayhead reads the same live 100 and resumeBoth seeks both elements there: a 30-second rewind over audio the viewer already heard, with slot 2 dragged backwards too.
Folding the record in with Math.max on the live branches — or clamping the assignment at line 361 to Math.max(ref ?? 0, playhead) while hidden — restores the invariant the comment claims. A plain Math.max in the foreground would be wrong, so it has to be gated on hidden, or done inside pairPlayhead where seekVideosTo's reset already protects it.
There was a problem hiding this comment.
Confirmed and fixed in bc4b21b. The comment on that assignment asserted the invariant rather than keeping it, which is the same failure mode as the drift guard that was written and never applied — so thanks for not taking it at its word.
Your sequence is right, and the un-paused-but-stalled half is the part I would not have reached on my own: paused === false genuinely cannot distinguish it, and this file already documents that as slot 1's normal failure mode.
Fixed inside pairPlayhead, where you suggested. Under trustSecondary no clock is authoritative any more, running ones included — every piece of evidence is weighed and the furthest wins. Without it the behaviour is exactly as before: a running slot 1 is the whole answer, so a scrub backwards is not dragged forward.
Fixing the read turned out not to be enough, and the test caught it. resumeBoth re-derived the position the foreground way and threw the recovered one away — slot 1 running at its frozen 100 gave 100 again on the resume path. It now takes the position as an argument and the reconcile hands over what it settled on; every other caller is unchanged.
I also kept your clamp at the call site (Math.max(ref, playhead) while hidden) as a local guard, so the invariant does not depend only on the helper holding it.
Three tests: the two helper cases (a record ahead of a running slot 1, and a stopped slot 2 ahead of one with no record), and the hook-level sequence — does not rewind to a stopped slot 1 that comes back un-paused behind slot 2. That one needed a browserResume on the test double, since the existing one could only model an element the browser stopped. All four fail against the previous commit.
| // storm; seeks on these cue-less WebM files are expensive (GEO-2828). And it never records a | ||
| // pause or an error on failure: refusing to start a video in a background tab is the | ||
| // browser's prerogative, not something the viewer needs to be told about. | ||
| if (hidden && turn) { |
There was a problem hiding this comment.
The hidden-tab restart retries forever on a platform that refuses off-screen starts, and each retry costs a parse-walk seek.
The if (hidden && turn) block writes speaking.currentTime and calls speaking.play() every MIN_BACKGROUND_RESTART_INTERVAL_MS (2s) for as long as the pair stays split. lastBackgroundRestartAtRef updates whether or not the play() resolves, so the throttle holds — but there is no attempt cap and no give-up.
Ticks keep arriving because the still-running element emits timeupdate (throttled, but roughly 1/s in a background tab), and playhead keeps advancing so the seek target keeps moving.
A viewer who unmutes a debate and leaves the tab backgrounded for 20 minutes on a browser that declines to start a <video> off screen gets roughly 600 currentTime writes — each a full demuxer parse walk on these cue-less MediaRecorder WebM files, per this file's own GEO-2828 comment — on an element that will never start.
A small consecutive-failure counter, reset on a successful play(), would bound it.
There was a problem hiding this comment.
Confirmed and fixed in bc4b21b. You are right that the floor paces the attempts without ever ending them, and that the ticks keep coming because the other element keeps emitting them.
Bounded as you suggested: five consecutive refusals and it stops asking. The counter clears when the speaking element is seen running — so a turn handing over to a healthy element resets it, and the budget is a run of refusals rather than a lifetime total — and again on the way back to visible, since a browser refusing off-screen starts is not refusing on-screen ones.
Five is enough to tell a slow start from a policy (two seconds apart, so ten seconds of trying), and the reconcile resumes the pair on return regardless, so nothing is lost by giving up early.
Test: gives up restarting an element the browser keeps refusing — twenty ticks, each past the floor, asserting at most five play() calls. Fails against the previous commit.
| const video = videoRef.current; | ||
| if (!video) return; | ||
| const wanted = audible ? 1 : 0; | ||
| video.volume = wanted; |
There was a problem hiding this comment.
The muted -> volume = 0 refactor probably does not buy what it is meant to buy, and the PR does not verify it.
The stated justification is that a browser is entitled to stop a <video> it considers silent once the tab is off screen, and that volume 0 is the same silence to a listener without being a mute.
In Blink that distinction does not exist where it matters: effective mute is muted || volume === 0, and that is what the autoplay and background-media heuristics read. So the listening element stays exactly as stoppable as it was, while now reporting as unmuted to the tab audio indicator and the media session.
This refactor is what pulls in the iOS read-only-volume probe (volumeIsWritable), the isResuming mute-repair handshake between the hook and the renderer, and the removal of the muted write from playFromStart — a lot of new coupling resting on a premise the PR's own "What I could not verify" section says was inferred rather than observed.
Worth confirming in a real backgrounded Chrome tab whether a volume: 0 element survives where a muted one does not. If it does not, the rest of the PR — the pairIsSettled stand-down and the visibilitychange reconcile — stands on its own without it.
There was a problem hiding this comment.
Reverted in bc4b21b. You are right, and I had reached the same reading of Blink's effective mute (muted || volume === 0) while writing it — then kept the refactor anyway on the grounds that it "costs nothing". It did not cost nothing: it pulled in the iOS read-only-volume probe, and made the listening element report as unmuted to the tab audio indicator and the media session for no benefit. Two independent readings of the same mechanism agreeing, with no evidence on the other side, is enough to cut it rather than defend it.
Per-turn audio is the muted flag again, exactly as on master. Gone with it: volumeIsWritable, the volume layout effect, and the three iOS tests.
Kept: the isResuming mute repair, because it is not part of this premise. playBothWithMutedFallback mutes both elements to retry a blocked play and leaves them muted when that fails too; React never repairs a DOM write it did not make, so the pair plays silently under a UI offering a mute control — true however muted is computed. Same reason playFromStart still does not write muted: the value it has is not the value rendered.
You are right that the rest stands on its own without it. What is left of the background half is the pairIsSettled stand-down, the visibilitychange reconcile, the playhead recovery, and the turn-boundary restart — and if the browser check comes back saying Chrome does not stop the silent element, the restart and recovery should go too. I have said as much on the PR; the check is the blocking item.
| }; | ||
| }, [isScrubbing, playbackEnded, playing, recoverBackgroundPlayhead, resumeBoth, userPaused]); | ||
|
|
||
| React.useEffect(() => { |
There was a problem hiding this comment.
Keying on visibility may not cover the reported repro, which the ticket describes as focus.
The ticket is "keep playback running when the browser window loses focus", and this deliberately switches to document.visibilityState.
On macOS, switching to another application leaves a non-occluded Chrome window visible, so documentIsHidden() stays false, pairIsSettled stays true, and the pre-existing split-pair path at line 451 fires unchanged — pausing the still-playing half and setting the sticky userPaused. If the reporter's repro is Cmd-Tab to another app rather than switching tabs or minimising, nothing here changes their experience.
One data point against the severity of this, measured on production master today: with document.visibilityState === "visible" and document.hasFocus() === false — the app-switch case — both elements of a pair kept playing normally across 10 samples at 500ms, advancing in real time. So focus loss alone did not trigger the split-pair pause there.
That makes the PR's suggested manual check the one that matters, and it should be a genuine tab switch or minimise rather than Cmd-Tab.
There was a problem hiding this comment.
Thank you for measuring this — it is the first real data anyone has put against the premise, and it changes what the manual check has to be. I have updated the PR description accordingly.
Taking the two halves separately:
Focus is the ticket's wording, visibility is the right key. The ticket's own "expected behavior" section separates "window or tab loses focus" from "video scrolls out of the viewport" and says the two are collapsed into one check today. document.hasFocus() is false for every window but the frontmost one, including a tab sitting fully on screen beside the one being typed in — keying on it would stop playback for a viewer who can still see the video. So I am keeping visibility, and your measurement supports that rather than undermining it.
But your measurement also says the app-switch case was never broken, which is the part that matters. If Cmd-Tab keeps both elements playing on production master, then either the reporter's repro is a genuine tab switch or minimise — which this PR does cover — or the bug is something else entirely and this PR does not fix it. I cannot tell from here, and neither can another review round.
So the check is now specified as: a genuine tab switch or minimise, not Cmd-Tab; unmuted; and long enough to cross a turn boundary, since the current-turn case and the boundary case fail differently. If Yaniv's original report was Cmd-Tab, that is worth knowing before this merges at all — it would mean the diagnosis is wrong from the first commit rather than incomplete.
One thing your data does settle: the split-pair path firing on focus loss is not what he hit, so the sticky-userPaused symptom must come from the hidden case if it comes from anywhere.
There was a problem hiding this comment.
Correcting myself on the reply above — I overstated this in two ways and Preston pushed back on it.
"The diagnosis is wrong from the first commit." It would not be. Your measurement covers focus loss with the window still visible; a hidden tab is a different browser state and nobody has measured it. If the repro was Cmd-Tab, the hidden-tab diagnosis could still be correct and simply not be what Yaniv hit. "Does not address the repro" is not "wrong".
"Should not merge as-is." Also too strong. Everything background-specific is gated on documentIsHidden() — in a visible window trustSecondary is off, the playhead clamp and the restart never run, and the reconcile only fires on a hidden→visible transition — so the speculative half is inert rather than risky. And the foreground changes fix bugs reachable on master today regardless of the premise: a cancelled resume no longer restarts playback the viewer paused, a blocked autoplay no longer leaves the pair silent under a UI offering a mute control, and the drift and split-pair corrections no longer fight a resume that is still confirming.
What the check actually decides is narrower: whether GEO-2947 can be closed (if the repro was Cmd-Tab, the symptom is untouched and the ticket should stay open rather than be closed by a green PR), and whether the ~200 lines of recovery and restart machinery should stay or be cut. The PR description now says that instead.
| // active speaker, and `playbackEnded`. | ||
| setPlayheadSeconds(recovered); | ||
|
|
||
| if (recovered < timelineSeconds - PLAYBACK_END_EPSILON_SECONDS) { |
There was a problem hiding this comment.
recoverBackgroundPlayhead and playbackEnded disagree at timelineSeconds === 0.
PLAYBACK_END_EPSILON_SECONDS was introduced so the two cannot disagree about whether a debate has finished, but playbackEnded (line 634) guards on timelineSeconds > 0 and this does not.
With timelineSeconds === 0 — a debate with empty turn_durations_ms and no turn_segments yet — recovered clamps to 0, 0 < -0.05 is false, and the reconcile takes the "finished while away" branch: setPlaying(false), no resume, and no userPaused, so showPausedGlyph gives the viewer no control back.
In practice updateTurnState's playhead >= timelineSeconds branch already stops a zero-length timeline before shouldBePlaying can be true, so I could not construct a reachable user-visible failure. Noting it because the guard is one token and the two checks are explicitly meant to agree.
There was a problem hiding this comment.
Fixed in bc4b21b — timelineSeconds <= 0 now short-circuits the recovery's end check, so it reads the same as playbackEnded's timelineSeconds > 0 guard.
Agreed it is not reachable today, for the reason you give, and I have not added a test for a state I cannot construct. Fixed anyway because the whole point of extracting the shared epsilon was that the two checks cannot disagree, and leaving one input where they still could makes the constant a half-truth.
… cut the volume gate **The recovered playhead could walk backwards.** `pairPlayhead` returned a running slot 1's clock before weighing anything else, so a browser that stopped slot 1 at debate-time 100 while slot 2 ran on to 130, then handed slot 1 back un-paused at its frozen 100 — un-suspended, or un-paused-but-stalled, which `paused === false` cannot tell apart — walked the record 130 -> 100 and resumed there, dragging slot 2 back over half a minute the viewer had already heard. Off screen no clock is authoritative now, running ones included: every piece of evidence is weighed and the furthest wins. The foreground path is untouched, where a running slot 1 is still the whole answer. Fixing the read was not enough: `resumeBoth` re-derived the position the foreground way and threw the recovered one away. It now takes the position as an argument, and the reconcile hands over what it settled on. Every other caller is unchanged. **The off-screen restart had no give-up.** The two-second floor paced the attempts but nothing ended them, and the ticks driving them keep arriving for as long as the other element plays. On a browser that declines to start a <video> off screen that is a `currentTime` write every two seconds for as long as the viewer is away — a demuxer parse walk each, on these cue-less files — on an element that will never start. Five consecutive refusals and it stops asking; a successful start, or coming back on screen, clears it. **The volume gate is reverted.** It rested on volume 0 being different from muted to a browser deciding what to stop off screen. In Blink effective mute is `muted || volume === 0`, so the listening element was exactly as stoppable as before while reporting as unmuted to the tab indicator and the media session — and it pulled in the iOS read-only-volume probe with it. Per-turn audio is the `muted` flag again, as on master. The `isResuming` repair stays: the autoplay fallback's mute is invisible to React however `muted` is computed. **`timelineSeconds === 0`** now reads the same to the recovery as it does to `playbackEnded`, which was the point of sharing the epsilon.
Patrick's review — all five addressed (bc4b21b)Replied on each thread. Summary, and one thing that changed the shape of the PR.
The one that changed the PRYou were right about Blink's effective mute being So the per-turn gate is the That is ~90 lines out of the production diff, and it is the first piece of the speculative half to go. The rest goes the same way if the browser check says the premise does not hold. The ratchet fix needed a second halfFixing Modelling it also needed a Your measurementThat is the first real data anyone has put against the premise, and it cuts both ways. It supports keying on visibility rather than focus — but it also says the app-switch case was never broken, so either the reporter's repro is a genuine tab switch or minimise, or the bug is something else and this PR does not fix it. The PR description now specifies the check as a real tab switch or minimise, unmuted, held past a turn boundary — and says plainly that if the original report was Cmd-Tab, the diagnosis is wrong from the first commit rather than incomplete, and this should not merge as-is. Verification4 new tests, all failing against the previous commit: two on the helper's ratchet, the hook-level un-paused-behind sequence, and the restart budget. Feed-player tests rewritten for the revert. 114 files / 1689 tests green. |
|
Heads-up on a collision, not a review point. #2449 ("fix(debates): videos stuck on Loading…, plus player control updates", @o-p-o-p-o, open since the 17th) touches the same two files as this PR, in overlapping places:
The Worth coordinating now rather than after review comments accumulate on both. No opinion from me on which should go first — #2449 is further along and not a draft, but this one is larger and has more review behind it. One substantive note while I'm here: #2449 fixes a card sitting on |
|
Correction to the above: I overstated the conflict. I reasoned from overlapping hunk ranges rather than trying the merge. Having now actually merged the two PR heads:
The rest stands: the two PRs are close together in the same files and worth coordinating, and #2449 fixes a distinct defect (a card stuck on |
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.
GEO-2947
Where the pause was coming from
The ticket's open question first: nothing in the playback path listens for
blurorvisibilitychange. I grepped every such listener inapps/web— they're all analytics, polling cadence, or the live debate room's tab-ownership handoff. None of them touch the feed player. So the pause was never ours to begin with; it was our reaction to one the browser made.A backgrounded tab is where a browser stops a
<video>it considers silent. The feed plays two elements and only the speaking debater's is audible (muted={!audible || mutedByUser}), so backgrounding stops the listening debater's element and leaves the speaker's running.updateTurnStateread that split pair as "the browser stopped playback on us" (the GEO-2783 autoplay-block path) and:userPaused, which is a sticky state the feed's auto-resume deliberately refuses to undo;playing, which dropsaudible, which re-appliesmutedto both elements.That is both halves of the report — "mutes audio and stops video" — plus the "only a click brings it back" that made it feel broken rather than merely idle.
The change
Two conditions that were collapsed into one are now separated, keyed on document visibility, not focus (a tab sitting on screen beside the window you're typing in never had a reason to stop):
resumeBoth, which re-seeks slot 2 to slot 1 first — so the pair comes back in step from where playback actually got to. No playhead reset, no change to the mute preference. A pair the browser let run through is not touched at all (no gratuitous seek).Scrolling a card out of the viewport is untouched — that still pauses, through
suspend, exactly as before.Guarded so it can't resume something the viewer didn't leave running: an explicit pause, a finished debate, or a scrub in progress all mean "leave it alone".
Open question from the ticket: two tabs both playing
Accepting it, not pausing other instances. That's what a background YouTube tab does, and cross-tab playback arbitration is a meaningfully larger piece of machinery (the debate room already has tab-ownership plumbing, and it's not cheap) for a case the viewer created deliberately. Happy to split it out if you'd rather we pause the others.
Tests
core/debates/use-debate-playback.test.tsx, 5 new cases — 2 regressions plus 3 controls. Verified they bite: with the source reverted to master and the tests kept, exactly the two regression cases fail and all three controls pass.userPaused✱core/debatesis green: 103 files / 1532 tests.tsc --noEmitreports errors in only the three files that fail on master.What I could not verify — and what the check has to be
I did not reproduce this in a real backgrounded tab. Playwright forces pages to stay visible (it passes
--disable-renderer-backgroundingand friends, and even with those strippeddocument.hiddennever flipped), and driving a real Chrome tab switch needs macOS accessibility permissions this environment doesn't have. So the diagnosis is from reading the code. It explains every part of the report — but the browser-side trigger is inferred, not observed.Patrick measured the missing half on production
master: withvisibilityState === "visible"andhasFocus() === false— switching to another application — both elements kept playing normally across 10 samples. So focus loss alone was never broken, and Cmd-Tab is not a valid repro for this ticket.That sharpens what to check before merge, on desktop Chrome and Safari:
What the check does and does not decide
It does not decide whether this merges. Everything background-specific is gated on
documentIsHidden(), so in a visible windowtrustSecondaryis off, the playhead clamp and the restart never run, and the reconcile only fires on a hidden→visible transition. The foreground changes stand on their own whatever the answer, and each fixes a bug reachable onmastertoday: a cancelled resume no longer restarts playback the viewer paused (isCancelled), a blocked autoplay no longer leaves the pair silent under a UI offering a mute control (theisResumingrepair), and the drift and split-pair corrections no longer fight a resume that is still confirming.What it decides is two other things:
pairIsSettledstand-down and thevisibilitychangereconcile stand without them.