From 4b5b66343ddf62b751443f253d25618a7fce8eb7 Mon Sep 17 00:00:00 2001 From: npeter83 Date: Sun, 25 Oct 2026 02:50:34 +0200 Subject: [PATCH] fix(player): related marks survive, Restart restarts, and refused pages offer nothing to click - Every state and Watch-later write goes through features/video/videoWrites, which marks the never-refetched lists (related lists, live searches) once the server has it; a contract test holds it. The Notification Center's undo, the feed's reset and the Watch-later toggles skipped it before. - Confirmed marks are also kept apart (qk.videoMarks) and laid over the related rows: a Show more page landing later rebuilt them from the pages it read when it started, and dropped a mark made meanwhile. - Restart on a related video starts at 0 (the session's stored position won before), and on the one already playing it seeks. - The watched toggle remembers each video's mark: Back showed the opened video's launch-time state after a related one was marked. - The help sheet leaves out the player's keys while a layer above it has them waiting. - After a refusal a retry cannot answer, neither Show more (related) nor Load more (live search) is offered; a failed live-search page keeps the results already read and says why under them. - The related cap message counts requests, as the cap does. --- .../components/ui/KeyboardHelp.dom.test.tsx | 28 ++++ frontend/src/components/ui/KeyboardHelp.tsx | 7 +- .../features/player/PlayerModal.dom.test.tsx | 93 +++++++++++++ frontend/src/features/player/PlayerModal.tsx | 37 ++++-- .../player/RelatedVideos.dom.test.tsx | 76 ++++++++++- .../src/features/player/RelatedVideos.tsx | 36 ++++-- .../video/videoWrites.contract.test.ts | 34 +++++ .../features/video/videoWrites.dom.test.ts | 47 +++++++ frontend/src/features/video/videoWrites.ts | 34 +++++ frontend/src/i18n/locales/en/errors.json | 2 +- frontend/src/i18n/locales/hu/errors.json | 2 +- frontend/src/lib/api.ts | 4 +- frontend/src/lib/api/core.ts | 6 + frontend/src/lib/keymap.ts | 16 ++- .../lib/playerModalHooks.apiload.dom.test.tsx | 1 + .../playerModalHooks.progress.dom.test.tsx | 1 + frontend/src/lib/playerModalHooks.ts | 8 +- frontend/src/lib/progressCache.test.ts | 38 ++++++ frontend/src/lib/progressCache.ts | 92 ++++++++----- frontend/src/lib/queryKeys.ts | 2 + .../modules/channels/WatchedVideosDialog.tsx | 7 +- frontend/src/modules/feed/Feed.tsx | 47 +++++-- .../modules/feed/Feed.ytLoadMore.dom.test.tsx | 122 ++++++++++++++++++ frontend/src/modules/home/Home.tsx | 9 +- .../notifications/NotificationsPanel.tsx | 4 +- frontend/src/modules/playlists/Playlists.tsx | 7 +- 26 files changed, 667 insertions(+), 93 deletions(-) create mode 100644 frontend/src/features/video/videoWrites.contract.test.ts create mode 100644 frontend/src/features/video/videoWrites.dom.test.ts create mode 100644 frontend/src/features/video/videoWrites.ts create mode 100644 frontend/src/lib/progressCache.test.ts create mode 100644 frontend/src/modules/feed/Feed.ytLoadMore.dom.test.tsx diff --git a/frontend/src/components/ui/KeyboardHelp.dom.test.tsx b/frontend/src/components/ui/KeyboardHelp.dom.test.tsx index c7b614e4..b9240155 100644 --- a/frontend/src/components/ui/KeyboardHelp.dom.test.tsx +++ b/frontend/src/components/ui/KeyboardHelp.dom.test.tsx @@ -9,6 +9,7 @@ import KeyboardHelp from "./KeyboardHelp"; import { useContextMenu } from "./useContextMenu"; import { closeKeyboardHelp, + setScopeCoveredProbe, useGlobalKeymap, useHelpConditions, useKeyScope, @@ -179,6 +180,33 @@ describe("KeyboardHelp", () => { expect(descriptions()).not.toContain("keys.contextMenu"); }); + // The YouTube player's keys wait while a layer sits above it (its related drawer, a dialog): a + // sheet opened there leaves them out, and one opened over the player itself lists them. + it("leaves out a surface's keys while a layer above it has them waiting", () => { + let covered = true; + setScopeCoveredProbe("ytPlayer", () => covered); + try { + render( + + + , + ); + const id = pushLayer(); + pressH(); + removeLayer(id); + expect(descriptions()).not.toContain("keys.playPause"); + expect(descriptions()).toContain("keys.help"); + act(() => closeKeyboardHelp()); + covered = false; + pressH(); + expect(descriptions()).toContain("keys.playPause"); + } catch (e) { + setScopeCoveredProbe("ytPlayer", null); + throw e; + } + setScopeCoveredProbe("ytPlayer", null); + }); + // Two live context menus (the Playlists page mounts one, its rail another) must not double the row. it("lists the context-menu chords once however many surfaces register", () => { function WithMenu() { diff --git a/frontend/src/components/ui/KeyboardHelp.tsx b/frontend/src/components/ui/KeyboardHelp.tsx index 6b509ed7..48adba47 100644 --- a/frontend/src/components/ui/KeyboardHelp.tsx +++ b/frontend/src/components/ui/KeyboardHelp.tsx @@ -10,6 +10,7 @@ import { useHelpConditionValues, useKeyboardHelpOpen, useKeyboardHelpOverLayer, + useKeyboardHelpCovered, useNavigablePages, } from "../../lib/keymap"; @@ -33,6 +34,7 @@ export default function KeyboardHelp() { const scopes = useActiveScopes(); const conditions = useHelpConditionValues(); const overLayer = useKeyboardHelpOverLayer(); + const covered = useKeyboardHelpCovered(); const pages = useNavigablePages(); if (!open) return null; @@ -47,7 +49,10 @@ export default function KeyboardHelp() { const inLayer = (s: (typeof scopes)[number]) => (!isPageLevelScope(s) && s !== "contextMenu") || (overLayer === "layerWithList" && (s === "list" || s === "contextMenu")); - const live = overLayer === "page" ? scopes : scopes.filter(inLayer); + // A surface under a layer of its own (the player under its related drawer) waits: not listed. + const live = (overLayer === "page" ? scopes : scopes.filter(inLayer)).filter( + (s) => !covered.has(s), + ); const lines = helpLines(live, pages).filter((l) => !l.when || !!conditions[l.when]); const words = { Space: t("keys.cap.space"), ContextMenu: t("keys.cap.menu") }; return ( diff --git a/frontend/src/features/player/PlayerModal.dom.test.tsx b/frontend/src/features/player/PlayerModal.dom.test.tsx index b34e895a..daf4d2bc 100644 --- a/frontend/src/features/player/PlayerModal.dom.test.tsx +++ b/frontend/src/features/player/PlayerModal.dom.test.tsx @@ -35,6 +35,8 @@ vi.mock("../downloads/DownloadButton", () => ({ })); import PlayerModal from "./PlayerModal"; +import KeyboardHelp from "../../components/ui/KeyboardHelp"; +import { closeKeyboardHelp, toggleKeyboardHelp } from "../../lib/keymap"; import { ConfirmProvider } from "../../components/ui/ConfirmProvider"; import { useConfirm } from "../../components/ui/confirmContext"; import { api, type Me, type Video } from "../../lib/api"; @@ -837,6 +839,7 @@ describe("PlayerModal — the side panel on a wide window", () => { const playRel2 = async ( onState: (id: string, status: string, v?: Video) => void, queue?: Video[], + opts: { onRemoveFromList?: (v: Video) => Promise; before?: () => void } = {}, ) => { wideWindow(true); vi.spyOn(api, "videoDetail").mockReturnValue(new Promise(() => {})); @@ -853,9 +856,11 @@ describe("PlayerModal — the side panel on a wide window", () => { startIndex={0} onClose={() => {}} onState={onState} + onRemoveFromList={opts.onRemoveFromList} />, ); act(() => playerEvents.onReady?.()); + opts.before?.(); fireEvent.click(screen.getByRole("tab", { name: "player.related.title" })); fireEvent.click(titleOf(await screen.findAllByRole("link", { name: "Video rel2" }))); await waitFor(() => expect(lastStart()?.videoId).toBe("rel2")); @@ -898,6 +903,77 @@ describe("PlayerModal — the side panel on a wide window", () => { expect(lastStart()?.videoId).toBe("rel2"); }); + // Restart is where the user asked to start: a position this session stored for the video (which a + // plain pick resumes from) must not win over it. + it("wide: Restart on a related video played this session starts it at 0", async () => { + vi.spyOn(api, "saveProgress").mockImplementation(async (id: string) => ({ + video_id: id, + position_seconds: id === "rel2" ? 500 : 77, + })); + clock.current = 77; + await playRel2(() => {}); + 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 row = within(list) + .getAllByRole("link", { name: "Video rel2" })[0]! + .closest("[data-nav-index]")!; + fireEvent.click(within(row as HTMLElement).getAllByTitle("card.restartTitle")[0]!); + await waitFor(() => expect(lastStart()?.videoId).toBe("rel2")); + // A start of 0 is no `start` at all (the player starts at the top). + 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 () => { + await playRel2(() => {}); + const before = started.length; + const list = screen.getByTestId("player-related"); + const row = within(list) + .getAllByRole("link", { name: "Video rel2" })[0]! + .closest("[data-nav-index]")!; + fireEvent.click(within(row as HTMLElement).getAllByTitle("card.restartTitle")[0]!); + expect(seeks).toContain(0); + expect(started.length).toBe(before); + }); + + // The toggle remembers each video's mark: a related one's did not overwrite the opened one's. + it("wide: Back shows the opened video's own mark, not the one it was opened with", async () => { + const onState = vi.fn(); + await playRel2(onState, undefined, { + before: () => fireEvent.click(screen.getAllByTitle("player.markWatched")[0]!), + }); + fireEvent.click(screen.getAllByTitle("player.markWatched")[0]!); // rel2's + expect(onState.mock.calls.map((c) => c.slice(0, 2))).toEqual([ + ["A", "watched"], + ["rel2", "watched"], + ]); + fireEvent.click(screen.getByRole("button", { name: "player.back" })); + await waitFor(() => expect(lastStart()?.videoId).toBe("A")); + expect(screen.getAllByTitle("player.watchedUnmark").length).toBeGreaterThan(0); + expect(screen.queryAllByTitle("player.markWatched")).toHaveLength(0); + }); + + it("wide: a queue step leaves the related video for the queue's next item", async () => { + await playRel2(() => {}, [vid("A", 40), vid("B", 0)]); + next(); + await waitFor(() => expect(lastStart()?.videoId).toBe("B")); + expect(screen.getByRole("heading", { name: "Video B" })).toBeTruthy(); + expect(screen.queryByRole("button", { name: "player.back" })).toBeNull(); + }); + + // Removing from the list removes the queue's item — not the related video on screen. + it("wide: offers no removal from the list while a related video plays", async () => { + await playRel2(() => {}, undefined, { onRemoveFromList: async () => true }); + expect(screen.queryByRole("button", { name: /playlists\.removeItemOf/ })).toBeNull(); + fireEvent.click(screen.getByRole("button", { name: "player.back" })); + await waitFor(() => expect(lastStart()?.videoId).toBe("A")); + expect(trash("A")).toBeTruthy(); + }); + it("leaves the related videos out when the instance turned them off", () => { me = { related_enabled: false } as Me; vi.spyOn(api, "videoDetail").mockReturnValue(new Promise(() => {})); @@ -1037,6 +1113,23 @@ describe("PlayerModal — the side panel on a wide window", () => { expect(seeks).toEqual([]); }); + // So the help sheet opened over the drawer does not promise them; over the player itself, it does. + it("stacked: the help sheet over the drawer leaves the video's keys out", async () => { + wideWindow(false); + const { drawer } = await openDrawer(); + render(); + act(() => toggleKeyboardHelp()); + expect(screen.queryByText("keys.playPause")).toBeNull(); + act(() => closeKeyboardHelp()); + fireEvent.click(within(drawer).getByRole("button", { name: "common.close" })); + await waitFor(() => + expect(screen.queryByRole("dialog", { name: "player.related.title" })).toBeNull(), + ); + act(() => toggleKeyboardHelp()); + expect(screen.getByText("keys.playPause")).toBeTruthy(); + act(() => closeKeyboardHelp()); + }); + it("stacked: the window regaining focus leaves it in the drawer", async () => { wideWindow(false); const { drawer } = await openDrawer(); diff --git a/frontend/src/features/player/PlayerModal.tsx b/frontend/src/features/player/PlayerModal.tsx index 9d2c0429..46866de7 100644 --- a/frontend/src/features/player/PlayerModal.tsx +++ b/frontend/src/features/player/PlayerModal.tsx @@ -65,7 +65,7 @@ import { relativeTime, } from "../../lib/format"; import { canFullscreenElements, fullscreenElement } from "../../lib/fullscreen"; -import { dispatch, isActivationTarget, useKeyScope } from "../../lib/keymap"; +import { dispatch, isActivationTarget, setScopeCoveredProbe, useKeyScope } from "../../lib/keymap"; import { useBackToClose } from "../../lib/history"; import ChannelNameLink from "../video/ChannelNameLink"; import Toaster from "../../components/ui/Toaster"; @@ -248,14 +248,18 @@ export default function PlayerModal({ // controls show — while the queue stays where it was, and Back returns to the opened item. It is // the SUBJECT of the player from here down; `active` stays the queue's item, which the related // list, the counter and prev/next belong to. A queue step leaves it. - const [related, setRelated] = useState<{ video: Video; startAt: number } | null>(null); + // `startAt` null resumes it where it was left (this session's stored position first); a number is + // where the user asked it to start — Restart's 0 — and beats that position. + const [related, setRelated] = useState<{ video: Video; startAt: number | null } | null>(null); const [relatedOf, setRelatedOf] = useState(active.id); if (relatedOf !== active.id) { setRelatedOf(active.id); setRelated(null); } const subject = related?.video ?? active; - const subjectResume = related ? related.startAt : resumeAt; + const subjectResume = related + ? (related.startAt ?? related.video.position_seconds ?? 0) + : resumeAt; const mountRef = useRef(null); const playerRef = useRef(null); // Keyboard/wheel controls. We keep focus on the modal card (not the cross-origin player @@ -547,14 +551,11 @@ export default function PlayerModal({ // item's, so the toggle falls back to `subject.status` and no ordering has to be argued about. // Unkeyed it landed last and painted the NEXT video as watched; a corrective click then sent // onState(next, "new") for an item that was already new. - const [status, setStatus] = useState<{ id: string; value: VideoStatus }>({ - id: active.id, - value: active.status, - }); - const watched = (status.id === subject.id ? status.value : subject.status) === "watched"; + const [marks, setMarks] = useState>({}); + const watched = (marks[subject.id] ?? subject.status) === "watched"; const setWatched = (on: boolean) => { const next = on ? "watched" : "new"; - setStatus({ id: subject.id, value: next }); + setMarks((m) => ({ ...m, [subject.id]: next })); onState(subject.id, next, subject); }; @@ -722,8 +723,14 @@ 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; - const playRelated = (v: Video, startAt: number) => { + 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 + // never be built (one per video), so a Restart is a seek, and a plain pick just keeps playing. + if (v.id === subject.id) { + if (startAt != null) seekTo(startAt); + return; + } setRelated({ video: v, startAt }); }; const descPop = useAnchoredPopover({ @@ -762,6 +769,15 @@ export default function PlayerModal({ }, []); useKeyScope("ytPlayer"); + // Its keys wait under a layer above it (the drawer, a dialog — see onPlayerKey), so the help sheet + // opened there must not list them. + useEffect(() => { + setScopeCoveredProbe( + "ytPlayer", + () => layerIdRef.current != null && anyBlockingLayerAbove(layerIdRef.current), + ); + return () => setScopeCoveredProbe("ytPlayer", null); + }, []); // Keyboard shortcuts. The chords live in the shared keymap (`ytPlayer` // scope) so the help sheet lists them; only the behaviour behind each id is here. @@ -888,6 +904,7 @@ export default function PlayerModal({ useYtProgress({ activeId: subject.id, resumeAt: subjectResume, + exactStart: related?.startAt != null, mountRef, playerRef, currentIdRef, diff --git a/frontend/src/features/player/RelatedVideos.dom.test.tsx b/frontend/src/features/player/RelatedVideos.dom.test.tsx index 3e8faaae..3b0374ed 100644 --- a/frontend/src/features/player/RelatedVideos.dom.test.tsx +++ b/frontend/src/features/player/RelatedVideos.dom.test.tsx @@ -25,6 +25,8 @@ import { RelatedVideos } from "./RelatedVideos"; import { api, HttpError, type RelatedVideo, type Video } from "../../lib/api"; import { clearAll, getNotifications } from "../../lib/notifications"; import { qk } from "../../lib/queryKeys"; +import { setHintsEnabled } from "../../lib/hints"; +import { writeVideoState } from "../video/videoWrites"; beforeAll(() => { class Stub { @@ -65,7 +67,7 @@ const page = (items: RelatedVideo[], next: string | null = null) => ({ }); function setup( - onPlay: (v: Video, startAt: number) => void = () => {}, + onPlay: (v: Video, startAt: number | null) => void = () => {}, onWrite?: (write: Promise) => void, ) { vi.spyOn(api, "comfortPins").mockResolvedValue({ items: [] } as never); @@ -116,12 +118,17 @@ describe("RelatedVideos", () => { expect(saved).toHaveBeenCalledWith({ relatedRegion: "hu" }); }); + // A plain pick resumes (null: the player knows the freshest position); the row's own position + // travels on the video it hands over. it("plays a row from where the user left it", async () => { vi.spyOn(api, "related").mockResolvedValue(page([row("A", { position_seconds: 95 })])); const onPlay = vi.fn(); setup(onPlay); fireEvent.click(await title("Video A")); - expect(onPlay).toHaveBeenCalledWith(expect.objectContaining({ id: "A" }), 95); + expect(onPlay).toHaveBeenCalledWith( + expect.objectContaining({ id: "A", position_seconds: 95 }), + null, + ); }); it("drops a video hidden here at once, and brings it back when the server refuses", async () => { @@ -308,6 +315,71 @@ describe("RelatedVideos", () => { expect(await screen.findByText("player.related.followed")).toBeTruthy(); }); + // An infinite query builds its next page from the pages it read when that fetch started: a mark + // that landed in between was dropped with them, for the session. + it("keeps a hide that landed while Show more was on its way", async () => { + let land: (p: ReturnType) => void = () => {}; + vi.spyOn(api, "related") + .mockResolvedValueOnce(page([row("A"), row("C")], "CUR1")) + .mockReturnValueOnce(new Promise((r) => (land = r)) as never); + const write = vi.spyOn(api, "setState").mockResolvedValue({ ok: true } as never); + setup(); + await title("Video A"); + fireEvent.click(screen.getByRole("button", { name: "player.related.more" })); + fireEvent.contextMenu((await title("Video A")).closest("[data-video-id]")!); + fireEvent.click(menuItem("card.hide")); + await waitFor(() => expect(write).toHaveBeenCalledWith("A", "hidden")); + await act(async () => {}); + await act(async () => land(page([row("B")]))); + await title("Video B"); + expect(titles()).toEqual(["Video C", "Video B"]); + }); + + // The Notification Center's undo, the feed's reset, a card elsewhere: every write reaches it. + it("follows a video's state written anywhere else", async () => { + vi.spyOn(api, "related").mockResolvedValue(page([row("A"), row("B")])); + vi.spyOn(api, "setState").mockResolvedValue({ ok: true } as never); + const { qc } = setup(); + await title("Video A"); + await act(async () => void (await writeVideoState(qc, "A", "hidden"))); + await waitFor(() => expect(titles()).toEqual(["Video B"])); + await act(async () => void (await writeVideoState(qc, "A", "new"))); + await waitFor(() => expect(titles()).toEqual(["Video A", "Video B"])); + }); + + // Show more is the request Retry would make: refused for good, it is not offered either. + it("offers no Show more once the next page was refused for good", async () => { + vi.spyOn(api, "related") + .mockResolvedValueOnce(page([row("A")], "CUR1")) + .mockRejectedValueOnce( + new HttpError(429, "Limit", { code: "refused", reason: "related_daily_limit", limit: 3 }), + ); + setup(); + await title("Video A"); + fireEvent.click(screen.getByRole("button", { name: "player.related.more" })); + expect((await screen.findByRole("alert")).textContent).toContain("Limit"); + expect(screen.queryByRole("button", { name: "player.related.more" })).toBeNull(); + expect(screen.queryByRole("button", { name: "common.retry" })).toBeNull(); + expect(titles()).toEqual(["Video A"]); + }); + + it("names the followed mark on hover with hints turned off too", async () => { + setHintsEnabled(false); + try { + vi.spyOn(api, "related").mockResolvedValue(page([row("A", { followed: true })])); + setup(); + await title("Video A"); + const mark = screen.getByRole("img", { name: "player.related.followed" }); + fireEvent.mouseEnter(mark.parentElement!); + fireEvent.pointerEnter(mark.parentElement!, { pointerType: "mouse" }); + expect(await screen.findByText("player.related.followed")).toBeTruthy(); + } catch (e) { + setHintsEnabled(true); + throw e; + } + setHintsEnabled(true); + }); + it("says when nothing is left that the user has not seen", async () => { vi.spyOn(api, "related").mockResolvedValue(page([])); setup(); diff --git a/frontend/src/features/player/RelatedVideos.tsx b/frontend/src/features/player/RelatedVideos.tsx index 9a08b6f4..031032b9 100644 --- a/frontend/src/features/player/RelatedVideos.tsx +++ b/frontend/src/features/player/RelatedVideos.tsx @@ -3,11 +3,18 @@ import { useTranslation } from "react-i18next"; import { useInfiniteQuery, useQueryClient } from "@tanstack/react-query"; import { CircleCheck } from "lucide-react"; -import { api, HttpError, type RelatedRegion, type Video, type VideoStatus } from "../../lib/api"; +import { + api, + HttpError, + refusalReason, + type RelatedRegion, + type Video, + type VideoStatus, +} from "../../lib/api"; import { qk } from "../../lib/queryKeys"; import { nextPageOnClick } from "../../lib/nextPage"; import { usePref, type UsePrefOptions } from "../../lib/usePref"; -import { patchRelated } from "../../lib/progressCache"; +import { useVideoMarks } from "../../lib/progressCache"; import { notifyActionError } from "../../lib/actionErrors"; import { Button } from "../../components/ui/Button"; import SegmentedToggle from "../../components/ui/SegmentedToggle"; @@ -18,6 +25,7 @@ import Tooltip from "../../components/ui/Tooltip"; import VideoCard from "../video/VideoCard"; import { VideoMenu } from "../video/VideoMenu"; import { announceVideoState } from "../video/stateNotice"; +import { writeVideoState, writeWatchLater } from "../video/videoWrites"; /** Which list YouTube fills in — on the video's topic or with what trends in Hungary — is a mood, * decided at the list, and the account keeps the last one (the owner's pick). */ @@ -33,9 +41,7 @@ const isSeen = (v: Video) => v.status === "watched" || v.status === "hidden"; /** The refusals a retry cannot answer: the day's cap (tomorrow does), the feature turned off on * this instance and the shared budget spent (the admin, or tomorrow). */ const FINAL_REASONS = new Set(["related_daily_limit", "related_disabled", "quota"]); -const isFinal = (e: unknown) => - e instanceof HttpError && - FINAL_REASONS.has((e.detailData as { reason?: string } | undefined)?.reason ?? ""); +const isFinal = (e: unknown) => FINAL_REASONS.has(refusalReason(e) ?? ""); // A write not yet confirmed: its change shows at once, and leaves when the server answers. type Pending = { seq: number; id: string; change: Partial