diff --git a/README.md b/README.md index c06d621..2d71786 100644 --- a/README.md +++ b/README.md @@ -395,6 +395,10 @@ Then put it behind your existing reverse proxy like everything else - [x] Linked whatever's playing (video or music) to the OS's own media controls (Windows' SMTC, macOS' Now Playing, ...) — title/artwork, play/pause/seek/next — see docs/architecture.md +- [x] Fixed switching a party between devices leaving both "on the + session" with no working controls — the old device never reacted + to being kicked, and the leave route had no way to tell a stale + device apart from the current one — see docs/architecture.md - [x] Fixed a track started directly (not via its album/artist) never falling back to its album's artwork — see docs/architecture.md (probably not the whole "images are hard to find" story — flagged diff --git a/client/src/api/party.ts b/client/src/api/party.ts index 86436a1..181cf67 100644 --- a/client/src/api/party.ts +++ b/client/src/api/party.ts @@ -39,8 +39,14 @@ export function joinParty(partyId: string): Promise { return apiFetch(`/api/parties/${encodeURIComponent(partyId)}/join`, { method: 'POST' }); } +/** Sends this device's own deviceId so the server can tell a stale + * device (already replaced by a takeover on another one - see + * getDeviceId's own comment) apart from the currently-active one, the + * same way the socket "join"/"leave"/"disconnect" handlers already do + * via the live connection itself - a REST call has no connection to + * compare, so this is what lets the server apply that same guard here. */ export function leaveParty(partyId: string): Promise<{ ok: true }> { - return apiFetch(`/api/parties/${partyId}/leave`, { method: 'POST' }); + return apiFetch(`/api/parties/${partyId}/leave`, { method: 'POST', body: JSON.stringify({ deviceId: getDeviceId() }) }); } export function getParty(partyId: string): Promise { diff --git a/client/src/musicBar/MusicBar.ts b/client/src/musicBar/MusicBar.ts index bf228e2..14bc293 100644 --- a/client/src/musicBar/MusicBar.ts +++ b/client/src/musicBar/MusicBar.ts @@ -266,7 +266,24 @@ export function joinMusicParty(partyId: string, party: PartyPublicState): void { else leaveMusicParty(); }, onError: (message) => showToast(message, { variant: 'error' }), - onClose: () => {} + // Fires for every disconnect, intentional or not - leaveMusicParty + // itself already clears activePartyId synchronously, well before its + // own conn.leave() actually closes the socket, so seeing it still set + // here means this device's connection died some other way (most + // often: replaced by a takeover on another device - see + // SocketGateway's "join" handler). Without this, the bar just sat + // there looking/sounding fully live forever on the old device, on a + // conn that was actually already dead underneath it - reported + // directly as "both devices end up on the session, controls never + // transfer." leaveMusicParty() is safe to call here even though the + // party already moved on: leaveParty's own deviceId check (see + // api/party.ts) makes its REST call a no-op for a stale device + // instead of ending the CURRENT device's membership by mistake. + onClose: () => { + if (!activePartyId) return; + showToast('Connexion à la party interrompue.', { variant: 'error' }); + leaveMusicParty(); + } }); } diff --git a/client/src/views/player/Player.ts b/client/src/views/player/Player.ts index a380a01..2b24964 100644 --- a/client/src/views/player/Player.ts +++ b/client/src/views/player/Player.ts @@ -229,6 +229,14 @@ function renderVideoPlayer(root: HTMLElement, partyId: string, party: PartyPubli let isHost = false; let hasNextQueued = false; + // Set at the top of cleanup() below, which every intentional way of + // leaving this route (the "Quitter" button, a sidebar link, browser + // back, ...) already runs synchronously before the socket actually + // closes - so by the time the async "disconnect" this connection's + // own onClose reacts to actually arrives, this is already true for + // every one of those. Still false there only means an unprompted + // close (most often: a device takeover elsewhere - see onClose below). + let leftIntentionally = false; let conn: PartyConnection | null = null; let hls: HlsInstance | null = null; let currentItemId: string | null = null; @@ -1277,7 +1285,23 @@ function renderVideoPlayer(root: HTMLElement, partyId: string, party: PartyPubli else navigate('/home'); }, onError: (message) => showToast(message, { variant: 'error' }), - onClose: () => {} + // Fires for every disconnect, intentional or not - leftIntentionally + // is already true by the time this runs for any of those (see its + // own comment), so still false here means an unprompted close, most + // often a device takeover elsewhere (SocketGateway's "join" handler) + // - without this, the video/hls.js instance just kept running + // forever on the old device, on a conn that was actually already + // dead underneath it, reported directly as "both devices end up on + // the session, controls never transfer." navigate() runs this same + // route's cleanup() below, which is now safe to do even though the + // party already moved on - leaveParty's own deviceId check (see + // api/party.ts) makes its REST call a no-op for a stale device + // instead of ending the CURRENT device's membership by mistake. + onClose: () => { + if (leftIntentionally) return; + showToast('Connexion à la party interrompue.', { variant: 'error' }); + navigate('/home'); + } }); } @@ -1291,6 +1315,7 @@ function renderVideoPlayer(root: HTMLElement, partyId: string, party: PartyPubli // other way (sidebar link, browser back, ...). This is the fix for the // actual leak: previously, only the "Quitter" button ran any of this. return function cleanup() { + leftIntentionally = true; if (heartbeat) clearInterval(heartbeat); if (progressTicker) clearInterval(progressTicker); if (requestBannerTimer) clearTimeout(requestBannerTimer); diff --git a/docs/architecture.md b/docs/architecture.md index 693e0e3..85fba14 100644 --- a/docs/architecture.md +++ b/docs/architecture.md @@ -4415,6 +4415,69 @@ call sites already gives), and the actual OS-level popup itself - the whole point - isn't something this sandbox can display or click regardless. Worth a real Windows/Chrome check once deployed. +## Device takeover: both devices "on the session", neither one's controls working + +Reported directly: switching a watch/listen party from one device to +another left both devices looking active, with no controls actually +transferring. The device-takeover mechanism itself - `SocketGateway`'s +"join" handler force-disconnecting the old device's socket once the new +one confirms - was already correct and already guarded against the +obvious failure mode (its own comment on the "leave"/"disconnect" +handlers: a stale connection's own eventual disconnect must not be +mistaken for a real leave). The actual bug was two gaps *around* that, +neither in the takeover mechanism itself. + +**Gap 1 - the old device never reacted to being kicked.** Both +`Player.ts` and `MusicBar.ts` wired `onClose: () => {}` - a complete +no-op - to the connection's "disconnect" event. So the moment a takeover +force-disconnected the old device's socket, that device's own video/ +hls.js instance (or the persistent music bar) just kept running exactly +as before: the socket was dead, but nothing told the local `