fix: review round 1 — row press after a rename, related reset, dead items on removal, honest Prev/Next
- A press that begins while a field in the row is edited only ends the editing (rowOpen latches it at pointerdown); the card layout's row press is now tested. - A related video reset in the player forgets the session's stored position. - Removing the playing video passes over known-dead items and steps the way it went. - Prev/Next off only because of dead items stay focusable and say why (HU/EN). - The docked bar's scroller leaves room for focus rings. - A focus-opened About card is not closed by its own scroll into view. - EN "Clear watch progress" for the video reset, distinct from a page's "Reset view". - The signed-out title read goes to the page yt-dlp read; the retitle writes are conditional. - Tests: the cap of three errors, the bar's own Fullscreen, the group glyph, the backfill's reads.
This commit is contained in:
@@ -1050,6 +1050,35 @@ describe("PlayerModal — the side panel on a wide window", () => {
|
||||
expect(lastStart()?.start ?? 0).toBe(0);
|
||||
});
|
||||
|
||||
// A reset from the related row is where it starts now: the position this session stored for it
|
||||
// (which a plain pick resumes from) is forgotten with it, not replayed and checkpointed back.
|
||||
it("wide: a related video reset from its row starts at 0 on a plain pick", async () => {
|
||||
vi.spyOn(api, "saveProgress").mockImplementation(async (id: string) => ({
|
||||
video_id: id,
|
||||
position_seconds: id === "rel2" ? 500 : 77,
|
||||
}));
|
||||
const reset = vi.spyOn(api, "clearState").mockResolvedValue(undefined as never);
|
||||
clock.current = 77;
|
||||
await playRel2(() => Promise.resolve());
|
||||
clock.current = 500;
|
||||
fireEvent.click(screen.getByRole("button", { name: "player.back" }));
|
||||
await waitFor(() => expect(lastStart()?.videoId).toBe("A"));
|
||||
await settle();
|
||||
const list = screen.getByTestId("player-related");
|
||||
const links = () => within(list).getAllByRole("link", { name: "Video rel2" });
|
||||
fireEvent.contextMenu(links()[0]!.closest("[data-video-id]")!);
|
||||
fireEvent.click(
|
||||
screen
|
||||
.queryAllByRole("menuitem", { hidden: true })
|
||||
.find((b) => b.textContent === "card.resetState")!,
|
||||
);
|
||||
await waitFor(() => expect(reset).toHaveBeenCalledWith("rel2"));
|
||||
await settle();
|
||||
fireEvent.click(titleOf(links()));
|
||||
await waitFor(() => expect(lastStart()?.videoId).toBe("rel2"));
|
||||
expect(lastStart()?.start ?? 0).toBe(0);
|
||||
});
|
||||
|
||||
// Its row stays in the list while it plays; a new player for the same video is never built (one
|
||||
// per video), so Restart there is a seek.
|
||||
it("wide: Restart on the related video already playing seeks it to 0", async () => {
|
||||
@@ -1654,9 +1683,10 @@ describe("PlayerModal — the docked bar's fullscreen", () => {
|
||||
it("keeps the way out in sight: glyphs only, and out of the scrolling part", () => {
|
||||
maximised(true);
|
||||
const out = screen.getByRole("button", { name: "player.exitMaximize" });
|
||||
const fulls = screen.getAllByLabelText("player.fullscreen");
|
||||
const full = fulls[fulls.length - 1]!;
|
||||
const bar = out.parentElement!;
|
||||
// The windowed panel keeps a Fullscreen of its own too; this is the bar's.
|
||||
const full = screen.getAllByLabelText("player.fullscreen").find((b) => bar.contains(b))!;
|
||||
expect(full).toBeTruthy();
|
||||
// The windowed row keeps its own Mark watched; this is the bar's.
|
||||
const watched = screen
|
||||
.getAllByRole("button", { name: "player.markWatched" })
|
||||
@@ -1898,15 +1928,88 @@ describe("PlayerModal — stepping over a video that cannot play", () => {
|
||||
expect(opened("B")).toBe(1);
|
||||
});
|
||||
|
||||
it("Previous is off when only dead videos are before you", async () => {
|
||||
// Off, and saying why: the counter shows a video before this one, so a dark button alone would not
|
||||
// tell (DESIGN "Unavailable with a reason stays focusable").
|
||||
it("Previous is off, with its reason, 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" }))
|
||||
const prevs = screen.getAllByRole("button", { name: "player.previous" });
|
||||
for (const b of prevs) {
|
||||
expect((b as HTMLButtonElement).disabled).toBe(false);
|
||||
expect(b.getAttribute("aria-disabled")).toBe("true");
|
||||
expect(b.getAttribute("title")).toContain("player.noPlayableBefore");
|
||||
}
|
||||
prev();
|
||||
await settle();
|
||||
expect(lastStart()?.videoId).toBe("B");
|
||||
// Next has playable C ahead: on, and no reason.
|
||||
for (const b of screen.getAllByRole("button", { name: "player.next" }))
|
||||
expect(b.getAttribute("title")).not.toContain("player.noPlayableAfter");
|
||||
});
|
||||
|
||||
it("Previous at the queue's start is plainly off: the counter says why", async () => {
|
||||
await open(0);
|
||||
for (const b of screen.getAllByRole("button", { name: "player.previous" })) {
|
||||
expect((b as HTMLButtonElement).disabled).toBe(true);
|
||||
expect(b.getAttribute("title")).not.toContain("player.noPlayableBefore");
|
||||
}
|
||||
});
|
||||
|
||||
// Removing the playing video passes over the ones known dead, the way a step does, and goes back
|
||||
// when nothing playable followed it.
|
||||
it("removing the last video goes back over a dead one to the one that plays", async () => {
|
||||
renderModal(
|
||||
<PlayerModal
|
||||
video={A}
|
||||
queue={[A, B, C]}
|
||||
startIndex={0}
|
||||
onClose={() => {}}
|
||||
onState={vi.fn()}
|
||||
onRemoveFromList={async () => true}
|
||||
/>,
|
||||
);
|
||||
await waitFor(() => expect(lastStart()?.videoId).toBe("A"));
|
||||
next();
|
||||
await waitFor(() => expect(lastStart()?.videoId).toBe("B"));
|
||||
fail();
|
||||
await waitFor(() => expect(lastStart()?.videoId).toBe("C"));
|
||||
fireEvent.click(trash("C"));
|
||||
await waitFor(() => expect(lastStart()?.videoId).toBe("A"));
|
||||
expect(opened("B")).toBe(1);
|
||||
});
|
||||
|
||||
// The player gives up after three in a row: it stays on the third's error, and all three are
|
||||
// remembered — a later step passes over them.
|
||||
it("three dead in a row stop on the third, and a step then passes over all three", async () => {
|
||||
const [D, E] = [vid("D", 0), vid("E", 0)];
|
||||
renderModal(
|
||||
<PlayerModal
|
||||
video={A}
|
||||
queue={[A, B, C, D, E]}
|
||||
startIndex={0}
|
||||
onClose={() => {}}
|
||||
onState={vi.fn()}
|
||||
/>,
|
||||
);
|
||||
await waitFor(() => expect(lastStart()?.videoId).toBe("A"));
|
||||
next();
|
||||
await waitFor(() => expect(lastStart()?.videoId).toBe("B"));
|
||||
fail();
|
||||
await waitFor(() => expect(lastStart()?.videoId).toBe("C"));
|
||||
fail();
|
||||
await waitFor(() => expect(lastStart()?.videoId).toBe("D"));
|
||||
fail();
|
||||
await settle();
|
||||
expect(lastStart()?.videoId).toBe("D");
|
||||
prev();
|
||||
await waitFor(() => expect(lastStart()?.videoId).toBe("A"));
|
||||
next();
|
||||
await waitFor(() => expect(lastStart()?.videoId).toBe("E"));
|
||||
expect([opened("B"), opened("C"), opened("D")]).toEqual([1, 1, 1]);
|
||||
});
|
||||
|
||||
it("a video opened dead with auto-advance off stays on its error", async () => {
|
||||
|
||||
@@ -15,6 +15,7 @@ import { useQuery, useQueryClient } from "@tanstack/react-query";
|
||||
import { qk } from "../../lib/queryKeys";
|
||||
import { usePref } from "../../lib/usePref";
|
||||
import { PlayerDescription, PlayerTransportControls } from "./playerModalControls";
|
||||
import { unavailable } from "../../components/ui/unavailable";
|
||||
import { AUTO_MODES, LOOP_MODES, type AutoMode, type LoopMode } from "../../lib/playerModes";
|
||||
import {
|
||||
AlertTriangle,
|
||||
@@ -702,11 +703,15 @@ export default function PlayerModal({
|
||||
const goNext = () => step(1);
|
||||
const canPrev = !!queue && stepIndex(queue, index, -1, dead) >= 0;
|
||||
const canNext = !!queue && stepIndex(queue, index, 1, dead) >= 0;
|
||||
// Off with items still left that way: only ones YouTube refused are there, which the counter alone
|
||||
// does not tell. At the queue's ends the counter does.
|
||||
const prevReason = queue && index > 0 ? t("player.noPlayableBefore") : undefined;
|
||||
const nextReason = queue && index < queue.length - 1 ? t("player.noPlayableAfter") : undefined;
|
||||
// "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
|
||||
// cannot tell. Step to the item that followed (clamped when the last one goes), and close when the
|
||||
// list runs out.
|
||||
// cannot tell. Step to the item that followed (the one before when the last one goes), passing over
|
||||
// the ones known unplayable, and close when the list runs out.
|
||||
//
|
||||
// Guarded against a double press by a REF, not by disabling the button: a trigger disabled for the
|
||||
// removal's lifetime was still disabled when focus came back to it (it once waited on a confirm) —
|
||||
@@ -723,19 +728,19 @@ export default function PlayerModal({
|
||||
// Decide from where the player is NOW, not where it was when the button was pressed: prev/next
|
||||
// and auto-advance keep working while the request runs, and a cursor from before the await
|
||||
// threw the viewer back onto a video they had already moved past.
|
||||
const { queue: q, index: i } = navRef.current;
|
||||
const { queue: q, index: i, dead: d } = navRef.current;
|
||||
if ((q?.[i] ?? video).id !== victim.id) {
|
||||
// They moved on — take the removed item out of the queue and leave playback where it is.
|
||||
setQueue((cur) => cur?.filter((v) => v.id !== victim.id));
|
||||
return;
|
||||
}
|
||||
const next = afterRemoval(q, i, victim.id);
|
||||
const next = afterRemoval(q, i, victim.id, d);
|
||||
if (!next) {
|
||||
onClose(); // that was the last one
|
||||
return;
|
||||
}
|
||||
setQueue(next.queue);
|
||||
stepTo(next.playingId, 1); // the item that followed: a dead one there carries on forward
|
||||
stepTo(next.playingId, next.way); // one that cannot play after all carries on the same way
|
||||
};
|
||||
|
||||
// Relative seek within the current video (plain Arrow keys), matching YouTube's ±5s.
|
||||
@@ -830,6 +835,9 @@ export default function PlayerModal({
|
||||
// Turned off on this instance (a daily limit of 0): no tab, no button — nothing that could only
|
||||
// be refused.
|
||||
const relatedOn = useMe().related_enabled;
|
||||
// A video reset from the related list starts over: the position this session stored for it would
|
||||
// otherwise resume a plain pick there, and the player's close would patch it back into the lists.
|
||||
const forgetPosition = (id: string) => void sessionPosRef.current.delete(id);
|
||||
const playRelated = (v: Video, startAt: number | null) => {
|
||||
setRelatedOpen(false);
|
||||
// The video already playing (its row stays in the list): a new player for the same id would
|
||||
@@ -1208,8 +1216,10 @@ export default function PlayerModal({
|
||||
>
|
||||
{/* Glyphs only, each named by its label and tooltip. The controls scroll when a
|
||||
narrow window cannot hold them; the way out (fullscreen, leave the big view)
|
||||
stays outside that part, in sight: on an iPad it had scrolled off the right. */}
|
||||
<div className="flex min-w-0 flex-1 items-center gap-x-3 overflow-x-auto">
|
||||
stays outside that part, in sight: on an iPad it had scrolled off the right.
|
||||
The bar's full height and a little side room: a scroller clips what overflows
|
||||
it, the focus ring drawn outside each control included. */}
|
||||
<div className="-mx-1 flex min-w-0 flex-1 items-center gap-x-3 self-stretch overflow-x-auto px-1">
|
||||
{hasQueue && (
|
||||
<>
|
||||
<PlayerTransportControls
|
||||
@@ -1218,6 +1228,8 @@ export default function PlayerModal({
|
||||
queueLength={queue!.length}
|
||||
canPrev={canPrev}
|
||||
canNext={canNext}
|
||||
prevReason={prevReason}
|
||||
nextReason={nextReason}
|
||||
autoMode={autoMode}
|
||||
loopMode={loopMode}
|
||||
onPrev={goPrev}
|
||||
@@ -1372,18 +1384,18 @@ export default function PlayerModal({
|
||||
<IconButton
|
||||
variant="ghost"
|
||||
onClick={goPrev}
|
||||
disabled={!canPrev}
|
||||
{...unavailable(!canPrev, prevReason)}
|
||||
label={t("player.previous")}
|
||||
title={`${t("player.previous")} · Shift+←`}
|
||||
title={`${t("player.previous")} · Shift+←${!canPrev && prevReason ? ` · ${prevReason}` : ""}`}
|
||||
>
|
||||
<SkipBack />
|
||||
</IconButton>
|
||||
<IconButton
|
||||
variant="ghost"
|
||||
onClick={goNext}
|
||||
disabled={!canNext}
|
||||
{...unavailable(!canNext, nextReason)}
|
||||
label={t("player.next")}
|
||||
title={`${t("player.next")} · Shift+→`}
|
||||
title={`${t("player.next")} · Shift+→${!canNext && nextReason ? ` · ${nextReason}` : ""}`}
|
||||
>
|
||||
<SkipForward />
|
||||
</IconButton>
|
||||
@@ -1419,6 +1431,8 @@ export default function PlayerModal({
|
||||
queueLength={queue!.length}
|
||||
canPrev={canPrev}
|
||||
canNext={canNext}
|
||||
prevReason={prevReason}
|
||||
nextReason={nextReason}
|
||||
autoMode={autoMode}
|
||||
loopMode={loopMode}
|
||||
onPrev={goPrev}
|
||||
@@ -1467,7 +1481,12 @@ export default function PlayerModal({
|
||||
maxWidth="max-w-sm"
|
||||
initialFocus="close"
|
||||
>
|
||||
<RelatedVideos videoId={active.id} onPlay={playRelated} onWrite={onWrite} />
|
||||
<RelatedVideos
|
||||
videoId={active.id}
|
||||
onPlay={playRelated}
|
||||
onWrite={onWrite}
|
||||
onReset={forgetPosition}
|
||||
/>
|
||||
</Modal>
|
||||
)}
|
||||
{(navigated || related) && (
|
||||
@@ -1723,6 +1742,7 @@ export default function PlayerModal({
|
||||
videoId={active.id}
|
||||
onPlay={playRelated}
|
||||
onWrite={onWrite}
|
||||
onReset={forgetPosition}
|
||||
/>
|
||||
</div>
|
||||
)}
|
||||
|
||||
@@ -54,12 +54,15 @@ export function RelatedVideos({
|
||||
videoId,
|
||||
onPlay,
|
||||
onWrite,
|
||||
onReset,
|
||||
}: {
|
||||
videoId: string;
|
||||
/** `startAt` null resumes the video where it was left; a number (Restart's 0) starts there. */
|
||||
onPlay: (v: Video, startAt: number | null) => void;
|
||||
/** Each write this list makes, for the host's player session (see PlayerModal `onWrite`). */
|
||||
onWrite?: (write: Promise<unknown>) => void;
|
||||
/** A video's view was reset from its row: the host forgets where its session left it. */
|
||||
onReset?: (id: string) => void;
|
||||
}) {
|
||||
const { t } = useTranslation();
|
||||
const qc = useQueryClient();
|
||||
@@ -107,6 +110,7 @@ export function RelatedVideos({
|
||||
};
|
||||
|
||||
const onResetState = (id: string) => {
|
||||
onReset?.(id);
|
||||
const seq = begin(id, { status: "new", position_seconds: 0 });
|
||||
const write = resetVideoState(qc, id);
|
||||
onWrite?.(write);
|
||||
|
||||
@@ -5,6 +5,7 @@ import type { VideoDetail } from "../../lib/api";
|
||||
import type { AutoMode, LoopMode } from "../../lib/playerModes";
|
||||
import { IconButton } from "../../components/ui/IconButton";
|
||||
import { StateMessage } from "../../components/ui/QueryState";
|
||||
import { unavailable } from "../../components/ui/unavailable";
|
||||
import { ClampedText } from "../../components/ui/ClampedText";
|
||||
import { renderDescription } from "./descriptionLinks";
|
||||
|
||||
@@ -28,6 +29,8 @@ export function PlayerTransportControls({
|
||||
queueLength,
|
||||
canPrev,
|
||||
canNext,
|
||||
prevReason,
|
||||
nextReason,
|
||||
autoMode,
|
||||
loopMode,
|
||||
onPrev,
|
||||
@@ -41,6 +44,11 @@ export function PlayerTransportControls({
|
||||
/** Whether there is a playable item that way (one YouTube refused is passed over). */
|
||||
canPrev: boolean;
|
||||
canNext: boolean;
|
||||
/** Why Previous / Next are off when items are left that way (only ones YouTube refused): the
|
||||
* button stays focusable and its tooltip says so. Unset at the queue's ends, which explain
|
||||
* themselves. */
|
||||
prevReason?: string;
|
||||
nextReason?: string;
|
||||
autoMode: AutoMode;
|
||||
loopMode: LoopMode;
|
||||
onPrev: () => void;
|
||||
@@ -62,6 +70,7 @@ export function PlayerTransportControls({
|
||||
title: string;
|
||||
onClick: () => void;
|
||||
disabled?: boolean;
|
||||
reason?: string;
|
||||
lit?: boolean;
|
||||
};
|
||||
const step: Control[] = [
|
||||
@@ -72,6 +81,7 @@ export function PlayerTransportControls({
|
||||
title: `${t("player.previous")} · Shift+←`,
|
||||
onClick: onPrev,
|
||||
disabled: !canPrev,
|
||||
reason: canPrev ? undefined : prevReason,
|
||||
},
|
||||
{
|
||||
key: "next",
|
||||
@@ -80,6 +90,7 @@ export function PlayerTransportControls({
|
||||
title: `${t("player.next")} · Shift+→`,
|
||||
onClick: onNext,
|
||||
disabled: !canNext,
|
||||
reason: canNext ? undefined : nextReason,
|
||||
},
|
||||
];
|
||||
// Persistent playback settings — cycle on click, saved to your account.
|
||||
@@ -107,10 +118,10 @@ export function PlayerTransportControls({
|
||||
key={c.key}
|
||||
variant={docked ? "overlay" : undefined}
|
||||
onClick={c.onClick}
|
||||
disabled={c.disabled}
|
||||
{...unavailable(c.disabled, c.reason)}
|
||||
lit={c.lit}
|
||||
label={c.name}
|
||||
title={c.title}
|
||||
title={c.reason ? `${c.title} · ${c.reason}` : c.title}
|
||||
>
|
||||
{c.glyph}
|
||||
</IconButton>
|
||||
|
||||
Reference in New Issue
Block a user