diff --git a/frontend/src/components/PlayerModal.dom.test.tsx b/frontend/src/components/PlayerModal.dom.test.tsx index f7aec589..201d86eb 100644 --- a/frontend/src/components/PlayerModal.dom.test.tsx +++ b/frontend/src/components/PlayerModal.dom.test.tsx @@ -8,8 +8,8 @@ // its promise in a module-level singleton, so this file must not share a module instance with a // test that leaves `window.YT` unset). Each item the modal plays constructs a new YT.Player, and the // fake records what it was asked for — that is how these tests see where playback starts. -import { describe, it, expect, vi, afterEach, beforeAll } from "vitest"; -import { render, screen, fireEvent, waitFor, cleanup } from "@testing-library/react"; +import { describe, it, expect, vi, afterEach, beforeAll, beforeEach } from "vitest"; +import { render, screen, fireEvent, waitFor, cleanup, act } from "@testing-library/react"; import { QueryClient, QueryClientProvider } from "@tanstack/react-query"; import type { ReactNode } from "react"; @@ -29,6 +29,7 @@ vi.mock("./AddToPlaylist", () => ({ default: () => null })); vi.mock("./DownloadButton", () => ({ default: () => null })); import PlayerModal from "./PlayerModal"; +import { ConfirmProvider, useConfirm } from "./ConfirmProvider"; import { api, type Video } from "../lib/api"; beforeAll(() => { @@ -113,3 +114,138 @@ describe("PlayerModal — where each item starts", () => { expect(lastStart()?.start).toBe(830); }); }); + +// --- "remove from this playlist", from inside the player ---------------------------------------- +const trash = (title: string) => + screen.getByRole("button", { name: `playlists.removeItemOf:Video ${title}` }); +const next = () => fireEvent.click(screen.getAllByRole("button", { name: "player.next" })[0]!); +const prev = () => fireEvent.click(screen.getAllByRole("button", { name: "player.previous" })[0]!); +const settle = () => act(async () => {}); + +describe("PlayerModal — removing the playing video from its playlist", () => { + const [A, B, C, D] = ["A", "B", "C", "D"].map((id) => vid(id, 0)); + beforeEach(() => { + vi.spyOn(api, "videoDetail").mockReturnValue(new Promise(() => {})); + vi.spyOn(api, "saveProgress").mockImplementation(async (id: string) => ({ + video_id: id, + position_seconds: 0, + })); + clock.current = 0; + }); + + it("steps to the item that followed, and the removed one is gone from the queue", async () => { + renderModal( + {}} + onState={() => {}} + onRemoveFromList={async () => true} + />, + ); + await waitFor(() => expect(lastStart()?.videoId).toBe("B")); + fireEvent.click(trash("B")); + await waitFor(() => expect(lastStart()?.videoId).toBe("C")); + prev(); // …straight back to A: B is no longer there to step onto + await waitFor(() => expect(lastStart()?.videoId).toBe("A")); + }); + + it("closes the player when the last item goes", async () => { + const onClose = vi.fn(); + renderModal( + {}} + onRemoveFromList={async () => true} + />, + ); + await waitFor(() => expect(lastStart()?.videoId).toBe("A")); + fireEvent.click(trash("A")); + await waitFor(() => expect(onClose).toHaveBeenCalled()); + }); + + it("leaves playback and the queue alone when the removal did not happen", async () => { + const onClose = vi.fn(); + renderModal( + {}} + onRemoveFromList={async () => false} + />, + ); + await waitFor(() => expect(lastStart()?.videoId).toBe("A")); + fireEvent.click(trash("A")); + await settle(); + expect(onClose).not.toHaveBeenCalled(); + expect(started.map((s) => s.videoId)).toEqual(["A"]); + next(); + await waitFor(() => expect(lastStart()?.videoId).toBe("B")); + prev(); // A is still in the queue + await waitFor(() => expect(lastStart()?.videoId).toBe("A")); + }); + + it("keeps playback where the viewer went while the confirm was open", async () => { + // prev/next keep working under the confirm dialog; deciding from the cursor as it was when the + // button was pressed threw the viewer back onto a video they had already moved past. + let answer!: (ok: boolean) => void; + renderModal( + {}} + onState={() => {}} + onRemoveFromList={() => new Promise((r) => (answer = r))} + />, + ); + await waitFor(() => expect(lastStart()?.videoId).toBe("A")); + fireEvent.click(trash("A")); + next(); + await waitFor(() => expect(lastStart()?.videoId).toBe("B")); + next(); + await waitFor(() => expect(lastStart()?.videoId).toBe("C")); + await act(async () => answer(true)); + expect(lastStart()?.videoId).toBe("C"); + prev(); + await waitFor(() => expect(lastStart()?.videoId).toBe("B")); + expect(started.filter((s) => s.videoId === "A")).toHaveLength(1); // never played again + }); + + it("gives focus back to the Remove button when the confirm is cancelled", async () => { + // The button used to be disabled for the confirm's whole lifetime, so the dialog's focus + // restore landed on a disabled element — a no-op — and a keyboard user was left on , + // outside the aria-modal player. + function WithConfirm() { + const confirm = useConfirm(); + return ( + {}} + onState={() => {}} + onRemoveFromList={(v) => + confirm({ title: "t", message: v.title, confirmLabel: "remove", danger: true }) + } + /> + ); + } + renderModal( + + + , + ); + await waitFor(() => expect(lastStart()?.videoId).toBe("A")); + trash("A").focus(); + fireEvent.click(trash("A")); + fireEvent.click(await screen.findByRole("button", { name: "common.cancel" })); + await waitFor(() => expect(document.activeElement).toBe(trash("A"))); + }); +}); diff --git a/frontend/src/components/PlayerModal.tsx b/frontend/src/components/PlayerModal.tsx index c79d679c..216ee9d4 100644 --- a/frontend/src/components/PlayerModal.tsx +++ b/frontend/src/components/PlayerModal.tsx @@ -120,7 +120,7 @@ export default function PlayerModal({ // useState's lazy initializer and ignore later prop changes. Items that turn out gone/ // non-embeddable still surface the existing playback-error overlay when reached. // - // The setter exists for exactly ONE writer: removing the playing video from its playlist (below). + // The setter exists for exactly ONE writer: removing a video from its playlist (below). // That is not the feed changing under us — it is this player's own action — and leaving the item // in the frozen queue would let prev/next step back onto a video that no longer exists. const [queue, setQueue] = useState(() => queueProp); @@ -262,20 +262,33 @@ export default function PlayerModal({ // parent owns the removal (confirm + API + caches); 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. Guarded against a double press while the confirm/request is in flight. - const [removing, setRemoving] = useState(false); + // list runs out. + // + // Guarded against a double press by a REF, not by disabling the button: a trigger disabled for + // the confirm's lifetime was still disabled when the dialog closed and restored focus to it — a + // no-op — so a keyboard user who cancelled was left on , outside this aria-modal player. + const removingRef = useRef(false); const removeFromList = async () => { - if (!onRemoveFromList || removing) return; + if (!onRemoveFromList || removingRef.current) return; const victim = active; - setRemoving(true); + removingRef.current = true; let done = false; try { done = await onRemoveFromList(victim); } finally { - setRemoving(false); + removingRef.current = false; } if (!done) return; // cancelled at the confirm, or the request failed — leave playback alone - const next = afterRemoval(queue, index, victim.id); + // Decide from where the player is NOW, not where it was when the button was pressed: prev/next + // and auto-advance keep working under the confirm dialog, 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; + 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); if (!next) { onClose(); // that was the last one return; @@ -905,10 +918,11 @@ export default function PlayerModal({ {!navigated && onRemoveFromList && ( diff --git a/frontend/src/components/Playlists.dom.test.tsx b/frontend/src/components/Playlists.dom.test.tsx index cf5ab858..db537359 100644 --- a/frontend/src/components/Playlists.dom.test.tsx +++ b/frontend/src/components/Playlists.dom.test.tsx @@ -8,6 +8,7 @@ import { render, screen, fireEvent, waitFor, cleanup, act } from "@testing-libra import { QueryClient, QueryClientProvider } from "@tanstack/react-query"; const h = vi.hoisted(() => ({ + notify: vi.fn(), selectedId: 1 as number | null, player: null as null | { video: { id: string; position_seconds: number }; @@ -26,6 +27,10 @@ vi.mock("react-i18next", async (importOriginal) => { }), }; }); +vi.mock("../lib/notifications", async (importOriginal) => ({ + ...(await importOriginal()), + notify: (...a: unknown[]) => h.notify(...a), +})); vi.mock("./PlaylistsProvider", () => ({ usePlaylists: () => ({ selectedId: h.selectedId }) })); vi.mock("./ConfirmProvider", () => ({ useConfirm: () => async () => true })); vi.mock("./usePlaylistActions", () => ({ usePlaylistActions: () => ({ remove: vi.fn() }) })); @@ -37,7 +42,7 @@ vi.mock("./PlayerModal", () => ({ })); import Playlists from "./Playlists"; -import { api, type Video } from "../lib/api"; +import { api, HttpError, type Video } from "../lib/api"; import { qk } from "../lib/queryKeys"; import { patchCachedProgress } from "../lib/progressCache"; @@ -87,11 +92,13 @@ function mount(lists: Record) { ); qc = new QueryClient({ defaultOptions: { queries: { retry: false } } }); qc.setQueryData(qk.me(), { id: 1, can_write: true, prefs: {} }); - return render( + const tree = () => ( - , + ); + const view = render(tree()); + return { rerender: () => view.rerender(tree()) }; } const play = async (title: string) => @@ -100,6 +107,7 @@ const play = async (title: string) => beforeEach(() => { h.selectedId = 1; h.player = null; + h.notify.mockClear(); }); afterEach(() => { cleanup(); @@ -122,3 +130,38 @@ describe("Playlists — the row the player is launched with", () => { await waitFor(() => expect(h.player?.video.position_seconds).toBe(830)); }); }); + +describe("Playlists — removing a video from inside the player", () => { + it("removes it from the list the player was launched from, even after the selection moved", async () => { + // The selection can change under an open player (the command palette opens over it); the + // removal read the LIVE selection, so it took the video out of a list the viewer never touched. + const remove = vi.spyOn(api, "removeFromPlaylist").mockResolvedValue(undefined as never); + const { rerender } = mount({ 1: [vid("V", 0)], 2: [vid("V", 0), vid("W", 0)] }); + await play("Video V"); + await waitFor(() => expect(h.player).not.toBeNull()); + h.selectedId = 2; + rerender(); + await screen.findByRole("button", { name: "card.play — Video W" }); // list 2 is on screen now + await act(async () => { + await h.player!.onRemoveFromList!(vid("V", 0)); + }); + expect(remove).toHaveBeenCalledWith(1, "V"); + // …and the list on screen keeps its own copy of V: nothing was taken out of it. + expect(screen.getByRole("button", { name: "card.play — Video V" })).toBeTruthy(); + }); + + it("says so when the removal fails because the list is gone", async () => { + // `req` leaves 403/404 to the caller; the catch assumed a global dialog had already reported + // it, so a confirmed removal failed in total silence behind the full-screen player. + vi.spyOn(api, "removeFromPlaylist").mockRejectedValue(new HttpError(404, "Unknown playlist")); + mount({ 1: [vid("V", 0)] }); + await play("Video V"); + await waitFor(() => expect(h.player).not.toBeNull()); + let done: boolean | undefined; + await act(async () => { + done = await h.player!.onRemoveFromList!(vid("V", 0)); + }); + expect(done).toBe(false); + expect(h.notify).toHaveBeenCalledWith(expect.objectContaining({ level: "error" })); + }); +}); diff --git a/frontend/src/components/Playlists.tsx b/frontend/src/components/Playlists.tsx index 197e14ce..61a0ce58 100644 --- a/frontend/src/components/Playlists.tsx +++ b/frontend/src/components/Playlists.tsx @@ -216,16 +216,23 @@ export default function Playlists() { // re-confirms our own reorder doesn't wipe the undo history — we reset history only when // the actual set of items changes (different playlist, or an add/remove). const lastSetRef = useRef(""); + // The selection as of NOW, for code that runs after an await or on behalf of the open player. + const selectedRef = useRef(selectedId); + selectedRef.current = selectedId; // What the player was LAUNCHED with, snapshotted when it opens: the video, the queue and the index // inside it. `shown` is derived (sort / filter / view), and the player freezes its queue and owns // its own cursor from mount — so deriving the mount from `shown` every render meant anything that // dropped the launch row yanked the player off the screen mid-playback. That was a latent edge // (a view change) until "remove this from the playlist" moved INSIDE the player, where it is the - // normal case. Only onClose takes the player down now. + // normal case. Only onClose takes the player down now. `listId` is part of that snapshot for the + // same reason: the selection can change under an open player (the command palette opens over it), + // and "remove this from the playlist" means the list it was LAUNCHED from, not whichever is + // selected by the time the button is pressed. const [playing, setPlaying] = useState<{ video: Video; queue: Video[]; index: number; + listId: number | null; } | null>(null); // Where the last move went. `LS.playlist` is the SELECTED (source) list and `updated_at` moves on // any edit, so neither answers "where do I keep sending things"; this does, and it is a per-account @@ -303,7 +310,7 @@ export default function Playlists() { const queue = shownRef.current; const index = queue.findIndex((v) => v.id === id); if (index < 0) return; - setPlaying({ video: queue[index]!, queue, index }); + setPlaying({ video: queue[index]!, queue, index, listId: selectedRef.current }); }, []); const onPlay = useCallback((v: Video) => openPlayer(v.id), [openPlayer]); const onRemove = useCallback((v: Video) => { @@ -312,8 +319,13 @@ export default function Playlists() { // The player's "remove from this playlist" goes through the SAME removeItem the row menu and the // inline ✕ use — one confirm, one optimistic update, one undo-stack rebase, no second copy to // drift. It answers whether the removal actually happened, because the player has to decide - // afterwards whether to step to the next item or close. - const removeFromPlayer = useCallback((v: Video) => removeRef.current(v), []); + // afterwards whether to step to the next item or close. It removes from the list the player was + // launched from (see `playing.listId`), not from whatever is selected by now. + const launchListId = playing?.listId ?? null; + const removeFromPlayer = useCallback( + (v: Video) => removeRef.current(v, launchListId), + [launchListId], + ); const storedIndex = useMemo(() => { const m = new Map(items.map((v, i) => [v.id, i])); @@ -399,9 +411,9 @@ export default function Playlists() { const plName = (p: { kind: string; name: string }) => playlistName(p, t); - function refreshAll() { + function refreshAll(listId: number | null = selectedId) { qc.invalidateQueries({ queryKey: qk.playlists() }); - qc.invalidateQueries({ queryKey: qk.playlist(selectedId) }); + qc.invalidateQueries({ queryKey: qk.playlist(listId) }); } async function revertToYoutube() { @@ -517,8 +529,11 @@ export default function Playlists() { /** @returns whether the item was actually removed — the player needs to know, because it must * step to the next item (or close) only when the removal really happened. */ - async function removeItem(video: Video): Promise { - if (selectedId == null) return false; + async function removeItem( + video: Video, + listId: number | null = selectedRef.current, + ): Promise { + if (listId == null) return false; const ok = await confirm({ title: t("playlists.removeItem"), message: t("playlists.confirmRemove", { title: video.title }), @@ -526,19 +541,28 @@ export default function Playlists() { danger: true, }); if (!ok) return false; - // A removal isn't itself undoable, but it no longer THROWS AWAY the reorder history: rebasing - // drops the item from every snapshot, so the stack stays valid for what's left. - const next = items.filter((v) => v.id !== video.id); - order.rebase((list) => list.filter((v) => v.id !== video.id)); - lastSetRef.current = `${selectedId}:${idsKey(next)}`; + // Only the list ON SCREEN has rows to take the item out of optimistically — the open player can + // be removing from the list it was launched from while another one is selected. Read after the + // confirm: the selection may have changed while it was open. + if (listId === selectedRef.current) { + // A removal isn't itself undoable, but it no longer THROWS AWAY the reorder history: rebasing + // drops the item from every snapshot, so the stack stays valid for what's left. + const next = order.value.filter((v) => v.id !== video.id); + order.rebase((list) => list.filter((v) => v.id !== video.id)); + lastSetRef.current = `${listId}:${idsKey(next)}`; + } try { - await api.removeFromPlaylist(selectedId, video.id); - refreshAll(); + await api.removeFromPlaylist(listId, video.id); + refreshAll(listId); return true; - } catch { - // The optimistic removal above didn't stick — resync from the server to restore the row. - // api.req already surfaced the error to the user via the global error dialog. - refreshAll(); + } catch (e) { + // `req` raises the global dialog for 4xx-validation and 5xx, but leaves 403/404 to the caller + // (the list was deleted or de-owned meanwhile) — so say it here, as moveItemTo does, or a + // confirmed removal fails in silence (inside the full-screen player there is not even a row + // snapping back to show it). + if (e instanceof HttpError && (e.status === 404 || e.status === 403)) + notify({ level: "error", message: e.detail || t("playlists.removeFailed") }); + refreshAll(listId); // …and resync, which restores any row the optimistic removal took out return false; } } diff --git a/frontend/src/i18n/locales/en/playlists.json b/frontend/src/i18n/locales/en/playlists.json index 8514f1f9..65f9fd5c 100644 --- a/frontend/src/i18n/locales/en/playlists.json +++ b/frontend/src/i18n/locales/en/playlists.json @@ -76,6 +76,7 @@ "moveNoTargets": "There is no other playlist to move it to yet.", "movedToast": "“{{title}}” moved to “{{name}}” ✓", "moveFailed": "Couldn't move the video.", + "removeFailed": "Couldn't remove the video.", "moveErrors": { "not_in_playlist": "That video isn't in this playlist any more — it changed somewhere else.", "same_playlist": "That video is already in this playlist." diff --git a/frontend/src/i18n/locales/hu/playlists.json b/frontend/src/i18n/locales/hu/playlists.json index 7998a421..a9bfe1f9 100644 --- a/frontend/src/i18n/locales/hu/playlists.json +++ b/frontend/src/i18n/locales/hu/playlists.json @@ -76,6 +76,7 @@ "moveNoTargets": "Még nincs másik lejátszási lista, ahová áthelyezhetnéd.", "movedToast": "„{{title}}” áthelyezve ide: „{{name}}” ✓", "moveFailed": "Nem sikerült áthelyezni a videót.", + "removeFailed": "Nem sikerült eltávolítani a videót.", "moveErrors": { "not_in_playlist": "Ez a videó már nincs ebben a listában — időközben máshol megváltozott.", "same_playlist": "Ez a videó már ebben a listában van."