From cc82626b537cdb837ae8f27e9e109572369f8ae1 Mon Sep 17 00:00:00 2001 From: Matt Toohey Date: Mon, 17 Aug 2026 22:20:05 +1000 Subject: [PATCH] fix(desktop): hand the media session to a video that is still playing MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Review follow-up to 3d94c0e. Ownership was last-claim-wins, but release cleared the session outright for the owner without considering that an earlier claimant might still be audible. Nothing in the app pauses one player when another starts, so two timeline videos can play at once: play A, play B, then let B unmount (scrolled out of the virtualized window, or its overlay closed). `releaseMediaSession(B)` set `metadata = null` and `playbackState = "none"` while A was still making noise, and A could not re-publish — `markMediaSessionPaused(A)` no-oped with `sessionOwner` back to null. Telling macOS there is no session while audio plays is exactly the state where the hardware key stops reaching the still-playing element, so this was worse than stale metadata. Claims now go into an ordered registry alongside the metadata each published, and losing ownership looks for a successor: the most recently claimed element that is still unpaused takes the session over, and it is cleared only when nothing else is audible. The owner *pausing* takes the same path — reporting "paused" over an audible sibling is the same lie in a milder form. Claimants that left the document without releasing are dropped during that scan, so the registry's strong keys cannot outlive the elements if some future caller claims without `useReleasingVideoRef`. Also makes the release-ordering test a real guard. It recorded `video.currentTime`, which is 0 in jsdom either way, so moving `onRelease` below `releaseVideoElement` left it green; it now records the source attribute, which reads back null once the release has detached it. ## Validation - `pnpm check`, `pnpm typecheck`, `pnpm test` (4961 desktop unit tests; the two `biome` warnings are pre-existing, in an unrelated E2E spec) - Mutation-checked the three new/changed tests against the old behaviour: clearing unconditionally fails both handover tests, and running `onRelease` after the detach fails the ordering test - Not covered: MediaRemote routing stays invisible to jsdom, so the outstanding manual macOS pass now also wants the two-video case — play one chat video, play a second, scroll the second away, and confirm the hardware key still controls the first rather than jumping back to Music Signed-off-by: Matt Toohey --- desktop/src/shared/ui/mediaSession.test.mjs | 112 ++++++++++++++++++-- desktop/src/shared/ui/mediaSession.ts | 83 ++++++++++++++- 2 files changed, 184 insertions(+), 11 deletions(-) diff --git a/desktop/src/shared/ui/mediaSession.test.mjs b/desktop/src/shared/ui/mediaSession.test.mjs index d9bac6a986..50aa0faae3 100644 --- a/desktop/src/shared/ui/mediaSession.test.mjs +++ b/desktop/src/shared/ui/mediaSession.test.mjs @@ -15,6 +15,7 @@ const dom = new JSDOM("", { const paused = []; const loaded = []; +const playingElements = new WeakSet(); let mediaSession; before(() => { @@ -24,10 +25,22 @@ before(() => { IS_REACT_ACT_ENVIRONMENT: true, window: dom.window, }); - // jsdom leaves the playback methods unimplemented; record the calls instead. + // jsdom leaves the playback methods unimplemented and hardcodes `paused` to + // true; record the calls and track playback ourselves instead, since who is + // still audible is what decides where the session goes. + dom.window.HTMLMediaElement.prototype.play = function play() { + playingElements.add(this); + }; dom.window.HTMLMediaElement.prototype.pause = function pause() { + playingElements.delete(this); paused.push(this); }; + Object.defineProperty(dom.window.HTMLMediaElement.prototype, "paused", { + configurable: true, + get() { + return !playingElements.has(this); + }, + }); dom.window.HTMLMediaElement.prototype.load = function load() { loaded.push(this); }; @@ -51,7 +64,14 @@ before(() => { afterEach(async () => { const { cleanup } = await import("@testing-library/react"); + const { releaseMediaSession } = await import("./mediaSession.ts"); cleanup(); + // The claimant registry is a module singleton, so hand-built elements have to + // be released or their ownership leaks into the next test. + for (const video of standaloneVideos.splice(0)) { + video.remove(); + releaseMediaSession(video); + } paused.length = 0; loaded.length = 0; mediaSession.metadata = null; @@ -60,6 +80,17 @@ afterEach(async () => { after(() => dom.window.close()); +/** Videos outside a React tree, in the document because a claimant that has + * left it is treated as gone. */ +const standaloneVideos = []; + +function appendVideo() { + const video = dom.window.document.createElement("video"); + dom.window.document.body.append(video); + standaloneVideos.push(video); + return video; +} + async function renderVideo({ strict = false } = {}) { const React = await import("react"); const { act, render } = await import("@testing-library/react"); @@ -68,7 +99,9 @@ async function renderVideo({ strict = false } = {}) { function Player() { const attach = useReleasingVideoRef({ - onRelease: (video) => released.push(video.currentTime), + // Recording the source pins the ordering: it reads back null once the + // release has detached it. + onRelease: (video) => released.push(video.getAttribute("src")), }); return React.createElement("video", { ref: attach, @@ -108,21 +141,24 @@ test("unmounting a player releases the media keys it claimed", async () => { assert.equal(mediaSession.playbackState, "none"); }); -test("release runs before the source is detached, so the position is still readable", async () => { +test("release runs before the source is detached, so the element is still readable", async () => { const { released, act, unmount } = await renderVideo(); await act(async () => unmount()); - assert.deepEqual(released, [0]); + // A callback running after the release would see a source-less element, and + // so could not read the playback position the inline player saves here. + assert.deepEqual(released, ["https://relay.example/media/clip.mp4"]); }); test("a bystander player's teardown leaves the playing video's session alone", async () => { const { claimMediaSession, releaseVideoElement } = await import( "./mediaSession.ts" ); - const playing = dom.window.document.createElement("video"); - const bystander = dom.window.document.createElement("video"); + const playing = appendVideo(); + const bystander = appendVideo(); + playing.play(); claimMediaSession(playing, { title: "review.mp4" }); // An off-screen timeline row unmounting while the review overlay plays. releaseVideoElement(bystander); @@ -131,6 +167,70 @@ test("a bystander player's teardown leaves the playing video's session alone", a assert.equal(mediaSession.playbackState, "playing"); }); +test("the owner's teardown hands the session to a player that is still audible", async () => { + const { claimMediaSession, markMediaSessionPaused, releaseVideoElement } = + await import("./mediaSession.ts"); + const first = appendVideo(); + const second = appendVideo(); + + // Nothing stops one player when another starts, so both are making noise and + // the later claim owns the session. + first.play(); + claimMediaSession(first, { artist: "engineering", title: "first.mp4" }); + second.play(); + claimMediaSession(second, { artist: "design", title: "second.mp4" }); + + // The owner scrolls out of the virtualized timeline. Reporting no session + // here would take the hardware key away from a video that is still playing. + releaseVideoElement(second); + + assert.equal(mediaSession.metadata.title, "first.mp4"); + assert.equal(mediaSession.playbackState, "playing"); + + // And the survivor owns the session, so its own pause is not a no-op. + first.pause(); + markMediaSessionPaused(first); + assert.equal(mediaSession.playbackState, "paused"); +}); + +test("the owner pausing hands the session to a player that is still audible", async () => { + const { claimMediaSession, markMediaSessionPaused } = await import( + "./mediaSession.ts" + ); + const first = appendVideo(); + const second = appendVideo(); + + first.play(); + claimMediaSession(first, { title: "first.mp4" }); + second.play(); + claimMediaSession(second, { title: "second.mp4" }); + + second.pause(); + markMediaSessionPaused(second); + + assert.equal(mediaSession.metadata.title, "first.mp4"); + assert.equal(mediaSession.playbackState, "playing"); +}); + +test("teardown with nothing else audible clears the session", async () => { + const { claimMediaSession, releaseVideoElement } = await import( + "./mediaSession.ts" + ); + const first = appendVideo(); + const second = appendVideo(); + + first.play(); + claimMediaSession(first, { title: "first.mp4" }); + second.play(); + claimMediaSession(second, { title: "second.mp4" }); + first.pause(); + + releaseVideoElement(second); + + assert.equal(mediaSession.metadata, null); + assert.equal(mediaSession.playbackState, "none"); +}); + test("StrictMode's simulated remount leaves the source attached", async () => { const { video } = await renderVideo({ strict: true }); diff --git a/desktop/src/shared/ui/mediaSession.ts b/desktop/src/shared/ui/mediaSession.ts index 9b9f4fc421..13359af558 100644 --- a/desktop/src/shared/ui/mediaSession.ts +++ b/desktop/src/shared/ui/mediaSession.ts @@ -12,7 +12,8 @@ import * as React from "react"; * works in every macOS app. Handing them back when the video goes away is what * this module exists for: pause, detach the source (the only thing that makes * WebKit destroy the session immediately rather than at some later GC), and - * clear the metadata this player published. + * clear the metadata this player published — or, if another player is still + * audible, hand the session to that one instead of claiming the page is quiet. */ export type NowPlayingMetadata = { @@ -28,6 +29,20 @@ export type NowPlayingMetadata = { */ let sessionOwner: HTMLVideoElement | null = null; +/** + * Every element that has claimed the session and not released it, in claim + * order, with the metadata each published. Nothing in the app stops one player + * when another starts, so several can be audible at once and the owner going + * away is not the same as the page going quiet — this is what the owner is + * handed over to instead. + * + * Strong keys, because picking a successor means iterating the claimants. Every + * element {@link useReleasingVideoRef} attaches is released when React drops + * it, which is what removes it here; {@link audibleClaimant} drops any that + * left the document some other way. + */ +const claimants = new Map(); + /** Sources detached by {@link releaseVideoElement}, for the StrictMode * re-attach path in {@link useReleasingVideoRef}. Weak so a released element * stays collectable. */ @@ -42,9 +57,7 @@ function getMediaSession(): MediaSession | null { return navigator.mediaSession ?? null; } -/** Publish what is playing so Control Center shows the video instead of a - * bare "Buzz" entry, and record this element as the session's owner. */ -export function claimMediaSession( +function publishNowPlaying( video: HTMLVideoElement, metadata: NowPlayingMetadata, ): void { @@ -62,19 +75,79 @@ export function claimMediaSession( session.playbackState = "playing"; } +/** + * The most recently claimed element other than `leaving` that is still making + * noise — the player the session belongs to once `leaving` stops owning it. + */ +function audibleClaimant( + leaving: HTMLVideoElement, +): { metadata: NowPlayingMetadata; video: HTMLVideoElement } | null { + let successor: { + metadata: NowPlayingMetadata; + video: HTMLVideoElement; + } | null = null; + for (const [video, metadata] of claimants) { + if (video === leaving) { + continue; + } + if (!video.isConnected) { + // Claimed, then removed from the document without a release: cannot be + // what the user is watching, and holding it here would leak the element. + claimants.delete(video); + continue; + } + if (!video.paused) { + successor = { metadata, video }; + } + } + return successor; +} + +/** Publish what is playing so Control Center shows the video instead of a + * bare "Buzz" entry, and record this element as the session's owner. */ +export function claimMediaSession( + video: HTMLVideoElement, + metadata: NowPlayingMetadata, +): void { + // Re-insert so claim order stays recency order: of two audible players, the + // one started last is the one the hardware key should reach. + claimants.delete(video); + claimants.set(video, metadata); + publishNowPlaying(video, metadata); +} + export function markMediaSessionPaused(video: HTMLVideoElement): void { + if (sessionOwner !== video) { + return; + } + // The owner pausing does not mean the page went quiet; report whatever is + // still audible rather than "paused" over the top of it. + const successor = audibleClaimant(video); + if (successor) { + publishNowPlaying(successor.video, successor.metadata); + return; + } const session = getMediaSession(); - if (!session || sessionOwner !== video) { + if (!session) { return; } session.playbackState = "paused"; } export function releaseMediaSession(video: HTMLVideoElement): void { + claimants.delete(video); if (sessionOwner !== video) { return; } sessionOwner = null; + const successor = audibleClaimant(video); + if (successor) { + // Reporting no session while another player is audible is exactly the + // state where the hardware key stops reaching it, so hand over instead of + // clearing — the leaving element's own session dies with its source. + publishNowPlaying(successor.video, successor.metadata); + return; + } const session = getMediaSession(); if (!session) { return;