From 15a36035139dbdf3d76d8f987c17fdd6b9a4b634 Mon Sep 17 00:00:00 2001 From: npeter83 Date: Sun, 1 Nov 2026 01:14:50 +0100 Subject: [PATCH] fix(player): a step onto an unplayable video carries on the way it went Previous onto an embed-blocked or removed video skipped forward again (the auto-advance direction), straight back to the item just left. The player now remembers the direction of the step that reached the item and the videos YouTube refused this session: an error carries on that way, Previous / Next pass over known-dead items without loading them, and go dark when only those are left. A step of your own skips with auto-advance off too. --- .../features/player/PlayerModal.dom.test.tsx | 84 +++++++++++++- frontend/src/features/player/PlayerModal.tsx | 107 ++++++++++++------ .../player/playerModalControls.dom.test.tsx | 2 + .../features/player/playerModalControls.tsx | 9 +- .../lib/playerModalHooks.apiload.dom.test.tsx | 3 +- .../playerModalHooks.progress.dom.test.tsx | 3 +- frontend/src/lib/playerModalHooks.ts | 30 +++-- frontend/src/lib/playerQueue.test.ts | 24 +++- frontend/src/lib/playerQueue.ts | 22 ++++ 9 files changed, 225 insertions(+), 59 deletions(-) diff --git a/frontend/src/features/player/PlayerModal.dom.test.tsx b/frontend/src/features/player/PlayerModal.dom.test.tsx index 2c2bbd90..ef439ef9 100644 --- a/frontend/src/features/player/PlayerModal.dom.test.tsx +++ b/frontend/src/features/player/PlayerModal.dom.test.tsx @@ -63,7 +63,11 @@ const started: { videoId: string; start: number | undefined }[] = []; // What the player was asked to switch to in place (a description link, a related video). const loaded: { videoId: string; startSeconds: number }[] = []; // The last player's event handlers, so a test can say "the player is ready" when it wants to. -let playerEvents: { onReady?: () => void; onStateChange?: (e: { data: number }) => void } = {}; +let playerEvents: { + onReady?: () => void; + onStateChange?: (e: { data: number }) => void; + onError?: (e: { data: number }) => void; +} = {}; const base = { getCurrentTime: () => clock.current, getDuration: () => clock.duration, @@ -1816,3 +1820,81 @@ describe("PlayerModal — clip mode", () => { expect(screen.getByTestId("clip-panel")).toBeTruthy(); }); }); + +// An embed-blocked or deleted video answers with onError, never 'ended'. Stepping BACK onto one used +// to skip forward again (the auto-advance direction), straight to the item you had just left. +describe("PlayerModal — stepping over a video that cannot play", () => { + const A = vid("A", 0); + const B = vid("B", 0); + const C = vid("C", 0); + const fail = () => act(() => playerEvents.onError?.({ data: 150 })); + const opened = (id: string) => started.filter((s) => s.videoId === id).length; + + beforeEach(() => { + vi.spyOn(api, "videoDetail").mockReturnValue(new Promise(() => {})); + vi.spyOn(api, "saveProgress").mockImplementation(async (id: string) => ({ + video_id: id, + position_seconds: 0, + })); + }); + + const open = async (startIndex: number) => { + renderModal( + {}} + onState={vi.fn()} + />, + ); + await waitFor(() => expect(lastStart()?.videoId).toBe([A, B, C][startIndex]!.id)); + }; + + it("Previous onto a dead video goes on back, not forward to where you came from", async () => { + me = { related_enabled: true, preferences: { playerAutoAdvance: "next" } } as unknown as Me; + await open(2); + prev(); + await waitFor(() => expect(lastStart()?.videoId).toBe("B")); + fail(); + await waitFor(() => expect(lastStart()?.videoId).toBe("A")); + }); + + it("a step of your own skips a dead video with auto-advance off too", async () => { + await open(2); + prev(); + await waitFor(() => expect(lastStart()?.videoId).toBe("B")); + fail(); + await waitFor(() => expect(lastStart()?.videoId).toBe("A")); + }); + + it("a video found dead is stepped over without loading it again", async () => { + await open(2); + prev(); + await waitFor(() => expect(lastStart()?.videoId).toBe("B")); + fail(); + await waitFor(() => expect(lastStart()?.videoId).toBe("A")); + next(); + await waitFor(() => expect(lastStart()?.videoId).toBe("C")); + expect(opened("B")).toBe(1); + }); + + it("Previous is off when only dead videos are before you", async () => { + await open(0); + fail(); // opened on it, auto-advance off: it stays, and it is known dead now + await settle(); + expect(lastStart()?.videoId).toBe("A"); + next(); + await waitFor(() => expect(lastStart()?.videoId).toBe("B")); + for (const b of screen.getAllByRole("button", { name: "player.previous" })) + expect((b as HTMLButtonElement).disabled).toBe(true); + }); + + it("a video opened dead with auto-advance off stays on its error", async () => { + await open(1); + fail(); + await settle(); + expect(lastStart()?.videoId).toBe("B"); + expect(started).toHaveLength(1); + }); +}); diff --git a/frontend/src/features/player/PlayerModal.tsx b/frontend/src/features/player/PlayerModal.tsx index ed6cd06a..d2bcd5ba 100644 --- a/frontend/src/features/player/PlayerModal.tsx +++ b/frontend/src/features/player/PlayerModal.tsx @@ -72,7 +72,7 @@ import { linkedPlayer, playerParamUpdates, type PlayerPanelTab } from "../../lib import ChannelNameLink from "../video/ChannelNameLink"; import Toaster from "../../components/ui/Toaster"; import { useOverlayOrder } from "../../components/ui/overlayOrder"; -import { afterRemoval } from "../../lib/playerQueue"; +import { afterRemoval, stepIndex } from "../../lib/playerQueue"; import { useNativeControlsYield, useYtBigView, @@ -118,6 +118,11 @@ function canScrollBy(el: HTMLElement | null, dir: -1 | 1): boolean { // a rem width, so it grows with the text size; the video takes what is left. const WIDE_PLAYER_PX = 1280; const SIDE_PANEL = "20rem"; +// The way a queue step goes: back, on, or to a random other item. +type Way = 1 | -1 | "random"; +const autoWay = (a: Exclude): Way => + a === "random" ? "random" : a === "next" ? 1 : -1; + // The side panel shut to a strip: one icon button wide, close / open / previous / next in a column. const PANEL_STRIP = "2.75rem"; // The narrowest the stacked video gets to make room for the chrome under it (see the card's maxWidth). @@ -657,15 +662,46 @@ export default function PlayerModal({ // Prev/next stepping through the queue (the feed's order, or a playlist). Read the live // queue+index from a ref so the window keydown handler (bound once) always steps from the // current position, not a stale closure. - const navRef = useLatest({ queue, index }); - const goPrev = () => { - const { queue: q, index: i } = navRef.current; - if (q && i > 0) setPlayingId(q[i - 1]!.id); + // + // The videos YouTube refused to play this session (embed-blocked, removed): a step passes over + // them, and Previous / Next go dark when only those are left that way. State, not a ref, because + // the buttons read it. + const [dead, setDead] = useState>(() => new Set()); + const navRef = useLatest({ queue, index, dead }); + // Which way the step that reached the playing item went — what an item that turns out unplayable + // carries on along. null: no step yet (the item the player opened on). + const stepRef = useRef<{ way: Way } | null>(null); + const stepTo = (id: string, way: Way) => { + stepRef.current = { way }; + setPlayingId(id); }; - const goNext = () => { - const { queue: q, index: i } = navRef.current; - if (q && i < q.length - 1) setPlayingId(q[i + 1]!.id); + // Step from `i` the given way over the videos known dead; whether there was anywhere to go. + const moveOn = ( + q: Video[], + i: number, + way: Way, + d: ReadonlySet, + wrap: boolean, + ): boolean => { + if (way === "random") { + const rest = q.filter((v, k) => k !== i && !d.has(v.id)); + if (!rest.length) return false; + stepTo(rest[Math.floor(Math.random() * rest.length)]!.id, "random"); + return true; + } + const j = stepIndex(q, i, way, d, wrap); + if (j < 0) return false; + stepTo(q[j]!.id, way); + return true; }; + const step = (dir: 1 | -1) => { + const { queue: q, index: i, dead: d } = navRef.current; + if (q) moveOn(q, i, dir, d, false); + }; + const goPrev = () => step(-1); + const goNext = () => step(1); + const canPrev = !!queue && stepIndex(queue, index, -1, dead) >= 0; + const canNext = !!queue && stepIndex(queue, index, 1, dead) >= 0; // "Remove from this playlist", offered right where you are watching the thing you want gone. The // parent owns the removal (API + caches + the toast whose Undo puts it back); the player owns what happens NEXT, because it // is the only side that knows which item is playing — its queue is frozen at mount, so the parent @@ -699,7 +735,7 @@ export default function PlayerModal({ return; } setQueue(next.queue); - setPlayingId(next.playingId); + stepTo(next.playingId, 1); // the item that followed: a dead one there carries on forward }; // Relative seek within the current video (plain Arrow keys), matching YouTube's ±5s. @@ -738,28 +774,28 @@ export default function PlayerModal({ if (l === "one") return replayCurrent(); // A related video's end is not the queue's: the list beside it is where the next one is picked. if (related) return; - const { queue: q, index: i } = navRef.current; - if (!q || q.length === 0) return; + const { queue: q, index: i, dead: d } = navRef.current; + if (!q || q.length === 0 || a === "off") return; const wrap = l === "all"; - if (a === "next") { - if (i < q.length - 1) setPlayingId(q[i + 1]!.id); - else if (wrap) { - if (q.length === 1) replayCurrent(); - else setPlayingId(q[0]!.id); - } - } else if (a === "prev") { - if (i > 0) setPlayingId(q[i - 1]!.id); - else if (wrap) { - if (q.length === 1) replayCurrent(); - else setPlayingId(q[q.length - 1]!.id); - } - } else if (a === "random" && q.length > 1) { - let r = i; - while (r === i) r = Math.floor(Math.random() * q.length); - setPlayingId(q[r]!.id); - } else if (a === "random" && wrap) { - replayCurrent(); - } + // Nowhere else to go (a one-item list, or every other item dead): Loop "All" repeats this one. + if (!moveOn(q, i, autoWay(a), d, wrap) && wrap) replayCurrent(); + }; + // The playing queue item turned out unplayable (YouTube's onError — it never reaches 'ended'). + // Remember it, then carry on the way the step that reached it went: Previous onto a dead video + // goes on back, not forward to the one you just left. The item the player opened on was reached + // by no step, so it moves on only with auto-advance on, in its direction. `go` false: the run of + // errors hit the player's cap — remembered, but playback stays on the error. + const skipUnplayable = (go: boolean) => { + if (related || clipOpenRef.current) return; + const { queue: q, index: i, dead: d } = navRef.current; + const gone = q?.[i]?.id; + if (!q || !gone) return; + const now = new Set(d).add(gone); + setDead(now); + if (!go || q.length <= 1) return; + const { autoMode: a, loopMode: l } = modeRef.current; + const way = stepRef.current?.way ?? (a === "off" ? null : autoWay(a)); + if (way != null) moveOn(q, i, way, now, l === "all"); }; // Lazy description (fetched only when it is opened). It opens from an ⓘ beside the title, by click @@ -982,14 +1018,13 @@ export default function PlayerModal({ mountRef, playerRef, currentIdRef, - navRef, - modeRef, setWatched, setPlayerError, setLiveData, reapplyStored, reclaimFocus, advanceOnEnd, + skipUnplayable, sessionPosRef, qc, }); @@ -1177,6 +1212,8 @@ export default function PlayerModal({ variant="docked" index={index} queueLength={queue!.length} + canPrev={canPrev} + canNext={canNext} autoMode={autoMode} loopMode={loopMode} onPrev={goPrev} @@ -1332,7 +1369,7 @@ export default function PlayerModal({ @@ -1341,7 +1378,7 @@ export default function PlayerModal({ = queue!.length - 1} + disabled={!canNext} label={t("player.next")} title={`${t("player.next")} · Shift+→`} > @@ -1377,6 +1414,8 @@ export default function PlayerModal({ variant="modal" index={index} queueLength={queue!.length} + canPrev={canPrev} + canNext={canNext} autoMode={autoMode} loopMode={loopMode} onPrev={goPrev} diff --git a/frontend/src/features/player/playerModalControls.dom.test.tsx b/frontend/src/features/player/playerModalControls.dom.test.tsx index 41cecc0f..f72b89da 100644 --- a/frontend/src/features/player/playerModalControls.dom.test.tsx +++ b/frontend/src/features/player/playerModalControls.dom.test.tsx @@ -24,6 +24,8 @@ function mount(variant: "modal" | "docked", autoMode: AutoMode, loopMode: LoopMo variant={variant} index={1} queueLength={3} + canPrev + canNext autoMode={autoMode} loopMode={loopMode} onPrev={() => {}} diff --git a/frontend/src/features/player/playerModalControls.tsx b/frontend/src/features/player/playerModalControls.tsx index e9e62378..9d5c62f9 100644 --- a/frontend/src/features/player/playerModalControls.tsx +++ b/frontend/src/features/player/playerModalControls.tsx @@ -27,6 +27,8 @@ export function PlayerTransportControls({ variant, index, queueLength, + canPrev, + canNext, autoMode, loopMode, onPrev, @@ -37,6 +39,9 @@ export function PlayerTransportControls({ variant: "docked" | "modal"; index: number; queueLength: number; + /** Whether there is a playable item that way (one YouTube refused is passed over). */ + canPrev: boolean; + canNext: boolean; autoMode: AutoMode; loopMode: LoopMode; onPrev: () => void; @@ -70,7 +75,7 @@ export function PlayerTransportControls({ name: t("player.previous"), title: `${t("player.previous")} · Shift+←`, onClick: onPrev, - disabled: index === 0, + disabled: !canPrev, labelFrom: "sm", }, { @@ -79,7 +84,7 @@ export function PlayerTransportControls({ name: t("player.next"), title: `${t("player.next")} · Shift+→`, onClick: onNext, - disabled: index === queueLength - 1, + disabled: !canNext, labelFrom: "sm", trailing: true, }, diff --git a/frontend/src/lib/playerModalHooks.apiload.dom.test.tsx b/frontend/src/lib/playerModalHooks.apiload.dom.test.tsx index 41acf1af..36ee92cc 100644 --- a/frontend/src/lib/playerModalHooks.apiload.dom.test.tsx +++ b/frontend/src/lib/playerModalHooks.apiload.dom.test.tsx @@ -34,14 +34,13 @@ function mountPlayer(setPlayerError: (code: number | null) => void) { mountRef: { current: mount }, playerRef: createRef() as RefObject, currentIdRef: { current: VID }, - navRef: { current: { queue: undefined, index: 0 } }, - modeRef: { current: { autoMode: "off", loopMode: "off" } }, setWatched: () => {}, setPlayerError, setLiveData: () => {}, reapplyStored: () => {}, reclaimFocus: () => {}, advanceOnEnd: () => {}, + skipUnplayable: () => {}, sessionPosRef: { current: new Map() }, qc: new QueryClient(), }), diff --git a/frontend/src/lib/playerModalHooks.progress.dom.test.tsx b/frontend/src/lib/playerModalHooks.progress.dom.test.tsx index 0f9aaa25..17331d38 100644 --- a/frontend/src/lib/playerModalHooks.progress.dom.test.tsx +++ b/frontend/src/lib/playerModalHooks.progress.dom.test.tsx @@ -76,14 +76,13 @@ function mountPlayer(qc: QueryClient, sessionPos: Map) { mountRef: { current: mount }, playerRef: createRef() as RefObject, currentIdRef: { current: VID }, - navRef: { current: { queue: undefined, index: 0 } }, - modeRef: { current: { autoMode: "off", loopMode: "off" } }, setWatched: () => {}, setPlayerError: () => {}, setLiveData: () => {}, reapplyStored: () => {}, reclaimFocus: () => {}, advanceOnEnd: () => {}, + skipUnplayable: () => {}, sessionPosRef: { current: sessionPos }, qc, }), diff --git a/frontend/src/lib/playerModalHooks.ts b/frontend/src/lib/playerModalHooks.ts index dfe5de55..e956bc83 100644 --- a/frontend/src/lib/playerModalHooks.ts +++ b/frontend/src/lib/playerModalHooks.ts @@ -17,12 +17,11 @@ import { requestFullscreen, } from "./fullscreen"; import { useBackToExit } from "./history"; -import { api, type Video } from "./api"; +import { api } from "./api"; import { qk } from "./queryKeys"; import { patchCachedProgress, trackProgressFlush } from "./progressCache"; import type { YTNamespace, YTPlayer, YTPlayerEvent } from "./youtube"; import { logYouTubeApiFailed, logYouTubeError, logYouTubeState } from "./mediaLog"; -import type { AutoMode, LoopMode } from "./playerModes"; // How close to the end (seconds) counts as "finished" → auto-mark watched. const FINISH_MARGIN = 10; @@ -334,7 +333,7 @@ function playerTimes(p: YTPlayer | null): { cur: number; dur: number } | null { /** Create the YouTube IFrame player for the active item, resume from the saved position, checkpoint * progress every 5 s (+ a pagehide beacon), and auto-mark watched once playback reaches the end. Keyed * on activeId (a queue advance recreates the player to reuse this single-video logic). The once-bound - * event closures read fresh queue/mode/volume via the refs passed in. Mirrors Plex's usePlexProgress — + * event closures read what must stay live via the refs passed in. Mirrors Plex's usePlexProgress — * side-effect only, returns nothing. */ export function useYtProgress({ activeId, @@ -343,14 +342,13 @@ export function useYtProgress({ mountRef, playerRef, currentIdRef, - navRef, - modeRef, setWatched, setPlayerError, setLiveData, reapplyStored, reclaimFocus, advanceOnEnd, + skipUnplayable, sessionPosRef, qc, }: { @@ -361,14 +359,14 @@ export function useYtProgress({ mountRef: RefObject; playerRef: RefObject; currentIdRef: RefObject; - navRef: RefObject<{ queue: Video[] | undefined; index: number }>; - modeRef: RefObject<{ autoMode: AutoMode; loopMode: LoopMode }>; setWatched: (on: boolean) => void; setPlayerError: (code: number | null) => void; setLiveData: (d: { title?: string; author?: string } | null) => void; reapplyStored: () => void; reclaimFocus: () => void; advanceOnEnd: () => void; + /** The item turned out unplayable; `go` false once a run of them hit the cap (stay on the error). */ + skipUnplayable: (go: boolean) => void; // Positions this player session has actually STORED, per video id. Written from the server's // answer, read back by the modal's resume so stepping next→prev inside one session does not // rewind to the value the frozen launch queue was snapshotted with. @@ -388,6 +386,7 @@ export function useYtProgress({ reapplyStored, reclaimFocus, advanceOnEnd, + skipUnplayable, qc, })); @@ -403,6 +402,7 @@ export function useYtProgress({ reapplyStored, reclaimFocus, advanceOnEnd, + skipUnplayable, qc, } = itemSnapshot(); // A position this session already stored for the item beats the queue's snapshot of it. @@ -492,16 +492,12 @@ export function useYtProgress({ onError: (e: YTPlayerEvent) => { logYouTubeError(typeof e?.data === "number" ? e.data : -1); setPlayerError(typeof e?.data === "number" ? e.data : -1); - // onError (not 'ended') fires for an unplayable item, so advanceOnEnd() never runs and an - // auto-advancing queue would freeze — skip forward instead, but bounded so a run of dead - // items can't spin. We do NOT mark the broken video watched. + // onError (not 'ended') fires for an unplayable item, so advanceOnEnd() never runs and the + // queue would freeze on it — the modal skips it the way the step that reached it went, + // bounded so a run of dead items can't spin. We do NOT mark the broken video watched. if (currentIdRef.current !== id) return; // a navigated description-link error: leave the queue alone - const { autoMode: a } = modeRef.current; - const { queue: q } = navRef.current; - if (a === "off" || !q || q.length <= 1) return; errorStreakRef.current += 1; - if (errorStreakRef.current >= MAX_CONSECUTIVE_ERRORS) return; // give up; leave the overlay - advanceOnEnd(); + skipUnplayable(errorStreakRef.current < MAX_CONSECUTIVE_ERRORS); }, }, }); @@ -565,9 +561,9 @@ export function useYtProgress({ // would be the bug, not the fix. `setWatched` is the clearest case: bound to the item, it lets // this cleanup's late auto-mark land on the item being LEFT, which the modal records for that // item and never paints on the next one. What genuinely has to stay live mid-session is behind - // refs (currentIdRef, modeRef, navRef, sessionPosRef, autoMarkedRef, errorStreakRef, playerRef) + // refs (currentIdRef, sessionPosRef, autoMarkedRef, errorStreakRef, playerRef) // — and THAT split is the enforcement: a value that is not a snapshot does not belong in this // closure. (This replaced an exhaustive-deps suppression, which made the React Compiler skip // the whole hook.) - }, [activeId, mountRef, playerRef, currentIdRef, navRef, modeRef, sessionPosRef]); + }, [activeId, mountRef, playerRef, currentIdRef, sessionPosRef]); } diff --git a/frontend/src/lib/playerQueue.test.ts b/frontend/src/lib/playerQueue.test.ts index 3add3a90..fdb19e8a 100644 --- a/frontend/src/lib/playerQueue.test.ts +++ b/frontend/src/lib/playerQueue.test.ts @@ -1,5 +1,5 @@ import { describe, it, expect } from "vitest"; -import { afterRemoval } from "./playerQueue"; +import { afterRemoval, stepIndex } from "./playerQueue"; import type { Video } from "./api"; // "Delete" from inside the player (the owner asked for it: today the only way to drop a video from a @@ -51,3 +51,25 @@ describe("afterRemoval", () => { expect(next!.playingId).toBe("a"); }); }); + +describe("stepIndex", () => { + const q = [v("a"), v("b"), v("c"), v("d")]; + const none = new Set(); + it("steps to the neighbour that way", () => { + expect(stepIndex(q, 1, 1, none)).toBe(2); + expect(stepIndex(q, 1, -1, none)).toBe(0); + }); + it("passes over a video known dead", () => { + expect(stepIndex(q, 3, -1, new Set(["c", "b"]))).toBe(0); + expect(stepIndex(q, 0, 1, new Set(["b"]))).toBe(2); + }); + it("is -1 at the end, or when only dead videos are left that way", () => { + expect(stepIndex(q, 3, 1, none)).toBe(-1); + expect(stepIndex(q, 2, -1, new Set(["a", "b"]))).toBe(-1); + }); + it("wraps when asked, and never lands on where it started", () => { + expect(stepIndex(q, 3, 1, none, true)).toBe(0); + expect(stepIndex(q, 0, -1, new Set(["d"]), true)).toBe(2); + expect(stepIndex(q, 1, 1, new Set(["a", "c", "d"]), true)).toBe(-1); + }); +}); diff --git a/frontend/src/lib/playerQueue.ts b/frontend/src/lib/playerQueue.ts index 2eac82ce..17c23f0e 100644 --- a/frontend/src/lib/playerQueue.ts +++ b/frontend/src/lib/playerQueue.ts @@ -28,3 +28,25 @@ export function afterRemoval( const next = rest[Math.min(Math.max(index, 0), rest.length - 1)]!; return { queue: rest, playingId: next.id }; } + +/** The nearest item from `index` going `dir` that is not known to be unplayable, or -1 when there is + * none that way. `wrap` goes round the ends (Loop "All"), never back onto `index` itself. + * + * A video YouTube refused to play (embed-blocked, removed) is remembered for the player session, so + * Previous / Next pass over it instead of loading it again only to be bounced off it. */ +export function stepIndex( + queue: readonly Video[], + index: number, + dir: 1 | -1, + dead: ReadonlySet, + wrap = false, +): number { + const n = queue.length; + for (let k = 1; k < n; k++) { + const raw = index + dir * k; + if (!wrap && (raw < 0 || raw >= n)) return -1; + const j = ((raw % n) + n) % n; + if (!dead.has(queue[j]!.id)) return j; + } + return -1; +}