fix(playlists): "remove" inside the player targets the list it came from, reports failure, and keeps the cursor and focus honest

Deep-review R5 — five defects in the Remove button f76a2afb put inside the YT player:

- It removed from whichever playlist was selected by the time it was pressed, not the one the player
  was launched from; the selection can change under an open player (the command palette opens over
  it). The launch list is now part of the player snapshot, and the optimistic row removal only
  touches the list actually on screen.
- A 403/404 failed in total silence: the catch claimed the global dialog had reported it, but `req`
  deliberately leaves those to the caller. It now notifies, as moveItemTo already did (the silent
  catch predates this round; the full-screen consumer did not). New key playlists.removeFailed, HU+EN.
- It computed the next item from the cursor as it was when the button was pressed, but prev/next and
  auto-advance keep working under the confirm dialog: the viewer was thrown back onto a video they had
  moved past. It reads the live cursor, and if they moved on it just drops the item from the queue.
- The button was disabled for the confirm's lifetime, so the dialog's focus restore hit a disabled
  element and a keyboard user who cancelled was left on <body>, outside the aria-modal player. A ref
  guards the double press instead.
- Its accessible name was a bare "Remove"; it now names its object, like the row control.

The call-site wiring (step on, drop from the queue, close on the last item, leave everything alone
when the removal did not happen) finally has tests. Verified on localdev in the real YT player: a row
reopened at the 136 s it had just stored; Cancel returned focus to the Remove button; a confirmed
removal stepped 1/3 -> 1/2. 10 mutations, each red on its assertion. Scratch list and positions reset.

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(
+      <PlayerModal
+        video={B!}
+        queue={[A!, B!, C!]}
+        startIndex={1}
+        onClose={() => {}}
+        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(
+      <PlayerModal
+        video={A!}
+        queue={[A!]}
+        startIndex={0}
+        onClose={onClose}
+        onState={() => {}}
+        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(
+      <PlayerModal
+        video={A!}
+        queue={[A!, B!]}
+        startIndex={0}
+        onClose={onClose}
+        onState={() => {}}
+        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(
+      <PlayerModal
+        video={A!}
+        queue={[A!, B!, C!, D!]}
+        startIndex={0}
+        onClose={() => {}}
+        onState={() => {}}
+        onRemoveFromList={() => new Promise<boolean>((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 <body>,
+    // outside the aria-modal player.
+    function WithConfirm() {
+      const confirm = useConfirm();
+      return (
+        <PlayerModal
+          video={A!}
+          queue={[A!, B!]}
+          startIndex={0}
+          onClose={() => {}}
+          onState={() => {}}
+          onRemoveFromList={(v) =>
+            confirm({ title: "t", message: v.title, confirmLabel: "remove", danger: true })
+          }
+        />
+      );
+    }
+    renderModal(
+      <ConfirmProvider>
+        <WithConfirm />
+      </ConfirmProvider>,
+    );
+    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")));
+  });
+});

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 <body>, 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 && (
                 <button
                   onClick={() => void removeFromList()}
-                  disabled={removing}
                   title={t("playlists.removeItem")}
-                  aria-label={t("playlists.removeItem")}
-                  className="shrink-0 inline-flex items-center justify-center w-9 h-9 rounded-lg text-muted hover:text-danger hover:bg-surface border border-border transition disabled:opacity-50"
+                  // Named with its object, like the row control: a bare "Remove" among Fullscreen /
+                  // Download / Mark watched reads as "delete this video".
+                  aria-label={t("playlists.removeItemOf", { title: active.title })}
+                  className="shrink-0 inline-flex items-center justify-center w-9 h-9 rounded-lg text-muted hover:text-danger hover:bg-surface border border-border transition"
                 >
                   <Trash2 className="w-4 h-4" />
                 </button>

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<typeof import("../lib/notifications")>()),
+  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<number, Video[]>) {
   );
   qc = new QueryClient({ defaultOptions: { queries: { retry: false } } });
   qc.setQueryData(qk.me(), { id: 1, can_write: true, prefs: {} });
-  return render(
+  const tree = () => (
     <QueryClientProvider client={qc}>
       <Playlists />
-    </QueryClientProvider>,
+    </QueryClientProvider>
   );
+  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" }));
+  });
+});

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<string>("");
+  // 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<boolean> {
-    if (selectedId == null) return false;
+  async function removeItem(
+    video: Video,
+    listId: number | null = selectedRef.current,
+  ): Promise<boolean> {
+    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;
+    // 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 = items.filter((v) => v.id !== video.id);
+      const next = order.value.filter((v) => v.id !== video.id);
       order.rebase((list) => list.filter((v) => v.id !== video.id));
-    lastSetRef.current = `${selectedId}:${idsKey(next)}`;
+      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;
     }
   }

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."

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."