From 7455fff401460beefa44297be3b217415ac7face Mon Sep 17 00:00:00 2001 From: npeter83 Date: Sat, 26 Sep 2026 04:05:39 +0200 Subject: [PATCH] =?UTF-8?q?feat(design):=20a=20playlist's=20one=20action?= =?UTF-8?q?=20list=20(C1)=20=E2=80=94=20the=20row's=20"=E2=80=A6",=20Delet?= =?UTF-8?q?e=20+=20undo,=20the=20header's=20"=E2=80=A6"=20(E28=20S3e2)?= MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit - Playlist rows: the X column became the row's "…" (play, pin, copy title/link, move, order, remove). Remove runs at once, with no confirm; its toast's Undo puts the row back at its index. The Delete key removes a keyboard-walked row, and focus moves on to the next. - POST /api/playlists/{id}/items takes an optional `position`, so an undo is one request that renumbers the list around the row. - A new `listDelete` key scope. Only a list with `onDelete` registers it, so the help sheet does not offer Delete where nothing handles it. - A playlist has one list, `usePlaylistActions.items`: rename, pin, YouTube open/push/revert, delete. The rail row's "…" and the page header's "…" both open it. Push and revert moved into the hook; the header keeps Play all. - Phone-width playlist table: "#" and Channel are hidden below sm (`Column.phoneHidden`). Their fixed widths had left the title no width. --- backend/app/routes/playlists.py | 29 +- backend/tests/test_playlist_add_position.py | 105 ++++++ frontend/src/components/PlayerModal.tsx | 14 +- .../src/components/Playlists.dom.test.tsx | 183 ++++++++++- frontend/src/components/Playlists.tsx | 311 ++++++------------ .../PlaylistsRail.rename.dom.test.tsx | 47 ++- frontend/src/components/PlaylistsRail.tsx | 128 ++++--- frontend/src/components/playlistColumns.tsx | 50 ++- frontend/src/components/ui/MoreButton.tsx | 5 + frontend/src/components/ui/NavGrid.tsx | 5 +- frontend/src/components/ui/tableHeader.tsx | 2 +- .../usePlaylistActions.dom.test.tsx | 78 ++++- frontend/src/components/usePlaylistActions.ts | 86 ----- .../src/components/usePlaylistActions.tsx | 293 +++++++++++++++++ frontend/src/i18n/locales/en/keys.json | 1 + frontend/src/i18n/locales/en/playlists.json | 3 +- frontend/src/i18n/locales/hu/keys.json | 1 + frontend/src/i18n/locales/hu/playlists.json | 3 +- frontend/src/lib/api/playlists.ts | 6 +- frontend/src/lib/keymap.ts | 15 +- frontend/src/lib/listNav.dom.test.tsx | 53 +++ frontend/src/lib/listNav.scope.dom.test.tsx | 20 +- frontend/src/lib/listNav.ts | 45 ++- frontend/src/lib/tableColumns.ts | 3 + 24 files changed, 1061 insertions(+), 425 deletions(-) create mode 100644 backend/tests/test_playlist_add_position.py delete mode 100644 frontend/src/components/usePlaylistActions.ts create mode 100644 frontend/src/components/usePlaylistActions.tsx diff --git a/backend/app/routes/playlists.py b/backend/app/routes/playlists.py index f4a09fb0..7ab2f2d9 100644 --- a/backend/app/routes/playlists.py +++ b/backend/app/routes/playlists.py @@ -4,7 +4,7 @@ mirror the user's own YouTube playlists in (`/sync-youtube`) and push/revert loc out (`/{id}/push-plan`, `/{id}/revert-youtube`), behind the optional write scope.""" from fastapi import APIRouter, Depends, HTTPException -from pydantic import BaseModel +from pydantic import BaseModel, Field from sqlalchemy import func, select, and_ from sqlalchemy.exc import IntegrityError from sqlalchemy.orm import Session, aliased @@ -507,17 +507,40 @@ def delete_playlist( return {"deleted": playlist_id} +class AddItemIn(VideoRefIn): + # Where the video lands among the list's items, in their stored order (0 = first). Omitted, it + # is appended. The list page's "Undo" of a removal puts the row back where it was with this — + # ONE request, because append-then-reorder leaves it at the end whenever the second call fails. + position: int | None = Field(default=None, ge=0) + + @router.post("/{playlist_id}/items") def add_item( playlist_id: int, - body: VideoRefIn, + body: AddItemIn, user: User = Depends(current_user), db: Session = Depends(get_db), ) -> dict: pl = owned_or_404(db, Playlist, playlist_id, user, what="playlist") video_id = body.video_id get_or_404(db, Video, video_id, what="video") - _add_item(db, pl, video_id, mark_dirty=True) + added = _add_item(db, pl, video_id, mark_dirty=False) + if added and body.position is not None: + # Renumber the whole list with the new row at `position` (clamped to the end): positions + # can have gaps after removals, so "index N" is only meaningful over the ordered rows. + rows = list( + db.execute( + select(PlaylistItem) + .where(PlaylistItem.playlist_id == pl.id) + .order_by(PlaylistItem.position) + ).scalars() + ) + new = next(r for r in rows if r.video_id == video_id) + rows.remove(new) + rows.insert(min(body.position, len(rows)), new) + for pos, row in enumerate(rows): + row.position = pos + recompute_dirty(db, pl) db.commit() return {"ok": True, "playlist_id": pl.id, "video_id": video_id} diff --git a/backend/tests/test_playlist_add_position.py b/backend/tests/test_playlist_add_position.py new file mode 100644 index 00000000..994774cb --- /dev/null +++ b/backend/tests/test_playlist_add_position.py @@ -0,0 +1,105 @@ +"""E28 S3e2 — adding a video AT a position: the list page's "Undo" of a removal. + +The row has to come back where it was in ONE request. The two-call shape (append, then send the +whole order) leaves it at the end of the list whenever the second call fails, and the reorder would +also overwrite any change another tab made in between. Positions can have gaps after removals, so +the position is an index over the ORDERED rows, and the list is renumbered around it. +""" + +from datetime import datetime, timezone + +from app.models import Channel, Playlist, PlaylistItem, Video + + +def _seed(db, user, ids, *, positions=None, **kw): + db.add(Channel(id="UCtest", title="Test")) + now = datetime.now(timezone.utc) + for v in [*ids, "vX"]: + db.add(Video(id=v, channel_id="UCtest", published_at=now)) + pl = Playlist(user_id=user.id, name="L", **kw) + db.add(pl) + db.flush() + for i, vid in enumerate(ids): + db.add(PlaylistItem(playlist_id=pl.id, video_id=vid, position=(positions or range(99))[i])) + db.commit() + return pl + + +def _ids(db, pl_id): + db.expire_all() + return [ + r.video_id + for r in db.execute( + PlaylistItem.__table__.select() + .where(PlaylistItem.playlist_id == pl_id) + .order_by(PlaylistItem.position) + ).all() + ] + + +def _add(client, pl_id, body): + return client.post(f"/api/playlists/{pl_id}/items", json=body) + + +def test_a_position_puts_the_video_back_in_its_place(db, make_user, as_user): + user = make_user() + pl = _seed(db, user, ["a", "b", "c"]) + r = _add(as_user(user), pl.id, {"video_id": "vX", "position": 1}) + assert r.status_code == 200 + assert _ids(db, pl.id) == ["a", "vX", "b", "c"] + + +def test_the_position_counts_rows_not_stored_numbers(db, make_user, as_user): + # A removal leaves a gap (0, 5, 9): index 2 is before the THIRD row, not "position 2". + user = make_user() + pl = _seed(db, user, ["a", "b", "c"], positions=[0, 5, 9]) + _add(as_user(user), pl.id, {"video_id": "vX", "position": 2}) + assert _ids(db, pl.id) == ["a", "b", "vX", "c"] + + +def test_a_position_past_the_end_appends(db, make_user, as_user): + user = make_user() + pl = _seed(db, user, ["a", "b"]) + _add(as_user(user), pl.id, {"video_id": "vX", "position": 40}) + assert _ids(db, pl.id) == ["a", "b", "vX"] + + +def test_no_position_still_appends(db, make_user, as_user): + user = make_user() + pl = _seed(db, user, ["a", "b"]) + _add(as_user(user), pl.id, {"video_id": "vX"}) + assert _ids(db, pl.id) == ["a", "b", "vX"] + + +def test_a_video_already_in_the_list_does_not_move(db, make_user, as_user): + # The add is idempotent; a position must not turn it into a reorder. + user = make_user() + pl = _seed(db, user, ["a", "b", "c"]) + _add(as_user(user), pl.id, {"video_id": "c", "position": 0}) + assert _ids(db, pl.id) == ["a", "b", "c"] + + +def test_a_negative_position_is_refused(db, make_user, as_user): + user = make_user() + pl = _seed(db, user, ["a"]) + r = _add(as_user(user), pl.id, {"video_id": "vX", "position": -1}) + assert r.status_code == 422 + assert _ids(db, pl.id) == ["a"] + + +def test_putting_a_removed_video_back_leaves_a_linked_list_clean(db, make_user, as_user): + # Remove + undo is no change at all: a YouTube-linked list must not stay "unsynced" after it. + user = make_user() + pl = _seed(db, user, ["a", "b", "c"], yt_playlist_id="PLx") + client = as_user(user) + from app.sync.playlists import current_fingerprint + + pl.synced_fingerprint = current_fingerprint(db, pl) + pl.dirty = False + db.commit() + client.delete(f"/api/playlists/{pl.id}/items/b") + db.expire_all() + assert db.get(Playlist, pl.id).dirty is True + _add(client, pl.id, {"video_id": "b", "position": 1}) + db.expire_all() + assert db.get(Playlist, pl.id).dirty is False diff --git a/frontend/src/components/PlayerModal.tsx b/frontend/src/components/PlayerModal.tsx index 72521add..6e3d42c5 100644 --- a/frontend/src/components/PlayerModal.tsx +++ b/frontend/src/components/PlayerModal.tsx @@ -308,14 +308,14 @@ export default function PlayerModal({ if (q && i < q.length - 1) setPlayingId(q[i + 1]!.id); }; // "Remove from this playlist", offered right where you are watching the thing you want gone. The - // parent owns the removal (confirm + API + caches); the player owns what happens NEXT, because it + // 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. // - // 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. + // 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) — + // a no-op — so a keyboard user was left on , outside this aria-modal player. const removingRef = useRef(false); const removeFromList = async () => { if (!onRemoveFromList || removingRef.current) return; @@ -327,9 +327,9 @@ export default function PlayerModal({ } finally { removingRef.current = false; } - if (!done) return; // cancelled at the confirm, or the request failed — leave playback alone + if (!done) return; // the request failed — leave playback alone // 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 + // 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; if ((q?.[i] ?? video).id !== victim.id) { @@ -967,7 +967,7 @@ export default function PlayerModal({ variant="outline-danger" size="md" onClick={() => void removeFromList()} - // Named with its object, like the row control: a bare "Remove" among Fullscreen / + // Named with its object, like the list's menu row: a bare "Remove" among Fullscreen / // Download / Mark watched reads as "delete this video". label={t("playlists.removeItemOf", { title: active.title })} title={t("playlists.removeItem")} diff --git a/frontend/src/components/Playlists.dom.test.tsx b/frontend/src/components/Playlists.dom.test.tsx index 43ae66a3..38151cd4 100644 --- a/frontend/src/components/Playlists.dom.test.tsx +++ b/frontend/src/components/Playlists.dom.test.tsx @@ -9,6 +9,7 @@ import { QueryClient, QueryClientProvider } from "@tanstack/react-query"; const h = vi.hoisted(() => ({ notify: vi.fn(), + confirm: vi.fn(async () => true), selectedId: 1 as number | null, mounted: false, mounts: 0, @@ -35,8 +36,18 @@ vi.mock("../lib/notifications", async (importOriginal) => ({ notify: (...a: unknown[]) => h.notify(...a), })); vi.mock("./playlistsContext", () => ({ usePlaylists: () => ({ selectedId: h.selectedId }) })); -vi.mock("./confirmContext", () => ({ useConfirm: () => async () => true })); -vi.mock("./usePlaylistActions", () => ({ usePlaylistActions: () => ({ remove: vi.fn() }) })); +vi.mock("./confirmContext", () => ({ useConfirm: () => h.confirm })); +// The list's own menu is usePlaylistActions' (pinned in its own test); here only its rename row, +// which opens THIS page's title field. +vi.mock("./usePlaylistActions", () => ({ + usePlaylistActions: () => ({ + remove: vi.fn(), + rename: vi.fn(), + items: (_pl: unknown, o: { onRename: () => void }) => [ + { id: "rename", label: "playlists.rename", onSelect: o.onRename }, + ], + }), +})); vi.mock("./PlayerModal", async () => { const { useEffect } = await import("react"); return { @@ -131,6 +142,7 @@ beforeEach(() => { h.mounted = false; h.mounts = 0; h.notify.mockClear(); + h.confirm.mockClear(); }); afterEach(() => { cleanup(); @@ -393,6 +405,173 @@ describe("Playlists — a change the server refused is taken back on screen", () }); }); +// E28 C1: a row's X column went. Remove is in the row's menu and on the Delete key, with no confirm — +// the toast's Undo puts the row back where it stood, in ONE request. +describe("Playlists — removing a row, and undoing it", () => { + // The server's side of a removal, so the refetch after it agrees with it. + const serverRemoves = (lists: Record) => + vi.spyOn(api, "removeFromPlaylist").mockImplementation(async (id: number, videoId: string) => { + lists[id] = lists[id]!.filter((v) => v.id !== videoId); + return undefined as never; + }); + // The removal's toast — found by what it says, not by being the latest: a refetch after the + // removal may notify too. + const lastToast = () => + h.notify.mock.calls + .map((c) => c[0] as { message: string; action?: { label: string; onClick: () => void } }) + .filter((n) => n.message?.startsWith("playlists.removedToast")) + .pop()!; + const pressKey = (key: string) => + act(() => { + (document.activeElement ?? document.body).dispatchEvent( + new KeyboardEvent("keydown", { key, bubbles: true, cancelable: true }), + ); + }); + + it("removes from the menu at once, and the toast's Undo puts it back at its place", async () => { + const lists = { 1: [vid("A", 0), vid("B", 0), vid("C", 0)] }; + serverRemoves(lists); + vi.spyOn(api, "addToPlaylist").mockResolvedValue(undefined as never); + mount(lists); + await rowMenu("Video B", "playlists.removeItem"); + await waitFor(() => expect(api.removeFromPlaylist).toHaveBeenCalledWith(1, "B")); + expect(rowsOnScreen()).toEqual(["Video A", "Video C"]); + // No confirm: the toast's Undo is the safety net. + expect(h.confirm).not.toHaveBeenCalled(); + await waitFor(() => expect(lastToast().message).toBe("playlists.removedToast:Video B")); + expect(lastToast().action?.label).toBe("common.undo"); + act(() => lastToast().action!.onClick()); + // Index 1 of the STORED order: where it was, not the end of the list. + await waitFor(() => expect(api.addToPlaylist).toHaveBeenCalledWith(1, "B", 1)); + }); + + it("puts it back where it stood in the stored order, whatever the view on screen", async () => { + vi.spyOn(api, "removeFromPlaylist").mockResolvedValue(undefined as never); + vi.spyOn(api, "addToPlaylist").mockResolvedValue(undefined as never); + mount({ 1: [vid("A", 0), vid("B", 0), vid("C", 0)] }); + await screen.findByRole("button", { name: "card.play — Video C" }); + // Sort the VIEW by title, descending: C, B, A on screen — the stored order is still A, B, C. + fireEvent.click(screen.getByRole("button", { name: /playlists\.colTitle/ })); + fireEvent.click(screen.getByRole("button", { name: /playlists\.colTitle/ })); + await waitFor(() => expect(rowsOnScreen()).toEqual(["Video C", "Video B", "Video A"])); + await rowMenu("Video A", "playlists.removeItem"); + await waitFor(() => expect(lastToast()?.action).toBeTruthy()); + act(() => lastToast().action!.onClick()); + await waitFor(() => expect(api.addToPlaylist).toHaveBeenCalledWith(1, "A", 0)); + }); + + it("says so when the Undo fails", async () => { + vi.spyOn(api, "removeFromPlaylist").mockResolvedValue(undefined as never); + vi.spyOn(api, "addToPlaylist").mockRejectedValue(new TypeError("Failed to fetch")); + mount({ 1: [vid("A", 0), vid("B", 0)] }); + await rowMenu("Video A", "playlists.removeItem"); + await waitFor(() => expect(lastToast()?.action).toBeTruthy()); + act(() => lastToast().action!.onClick()); + await waitFor(() => + expect(h.notify).toHaveBeenCalledWith( + expect.objectContaining({ level: "error", title: "playlists.undoRemoveFailed" }), + ), + ); + }); + + it("removes the walked row on Delete, and the focus goes on to the next", async () => { + const lists = { 1: [vid("A", 0), vid("B", 0), vid("C", 0)] }; + serverRemoves(lists); + mount(lists); + await screen.findByRole("button", { name: "card.play — Video C" }); + pressKey("j"); + pressKey("j"); + expect(document.activeElement?.textContent).toContain("Video B"); + pressKey("Delete"); + await waitFor(() => expect(api.removeFromPlaylist).toHaveBeenCalledWith(1, "B")); + expect(rowsOnScreen()).toEqual(["Video A", "Video C"]); + await waitFor(() => expect(document.activeElement?.textContent).toContain("Video C")); + }); + + it("offers no Delete, no Remove and no X on Watch later's rows", async () => { + vi.spyOn(api, "removeFromPlaylist").mockResolvedValue(undefined as never); + h.selectedId = WATCH_LATER; + mount({ [WATCH_LATER]: [vid("A", 0), vid("B", 0)] }); + await screen.findByRole("button", { name: "card.play — Video B" }); + pressKey("j"); + pressKey("Delete"); + await act(async () => void (await new Promise((r) => setTimeout(r, 20)))); + expect(api.removeFromPlaylist).not.toHaveBeenCalled(); + expect(screen.queryByRole("button", { name: /removeItemOf/ })).toBeNull(); + }); + + it("opens the row's menu from its '…', with the copy rows", async () => { + mount({ 1: [vid("A", 0)] }); + await screen.findByRole("button", { name: "card.play — Video A" }); + const more = screen.getByRole("button", { name: "playlists.rowMenu" }); + expect(more.getAttribute("aria-expanded")).toBe("false"); + fireEvent.click(more); + await waitFor(() => + expect(screen.getAllByRole("menuitem", { hidden: true }).map((b) => b.textContent)).toEqual( + expect.arrayContaining([ + "card.play", + "common.copy.title", + "common.copy.link", + "playlists.removeItem", + ]), + ), + ); + expect( + screen.getByRole("button", { name: "playlists.rowMenu" }).getAttribute("aria-expanded"), + ).toBe("true"); + }); +}); + +// E28 C1: the header keeps Play all; rename, YouTube and delete are the list's menu, behind a "…". +describe("Playlists · the header's '…'", () => { + it("replaces the pencil, the YouTube buttons and the trash, and renames from its menu", async () => { + mount({ 1: [vid("A", 0)] }); + await screen.findByRole("button", { name: "card.play — Video A" }); + for (const gone of [ + "playlists.rename", + "playlists.delete", + "playlists.pushToYoutube", + "playlists.exportToYoutube", + "playlists.openOnYoutube", + ]) + expect(screen.queryByRole("button", { name: gone })).toBeNull(); + const more = screen.getByRole("button", { name: "playlists.railMenu" }); + act(() => more.focus()); + fireEvent.click(more); + // The surface is visibility:hidden in jsdom (floating-ui never positions): matched on text. + const renameRow = await waitFor(() => { + const row = screen + .getAllByRole("menuitem", { hidden: true }) + .find((b) => b.textContent === "playlists.rename"); + expect(row).toBeTruthy(); + return row!; + }); + fireEvent.click(renameRow); + const field = await screen.findByRole("textbox", { name: "playlists.rename" }); + // Leaving the field by Escape hands the focus back to the "…" it was opened from. + fireEvent.keyDown(field, { key: "Escape" }); + await waitFor(() => + expect(document.activeElement).toBe( + screen.getByRole("button", { name: "playlists.railMenu" }), + ), + ); + }); +}); + +describe("Playlists · a phone-width table", () => { + it("leaves out '#' and Channel below sm, header and cells alike, so the title keeps a width", async () => { + mount({ 1: [vid("A", 0)] }); + await screen.findByRole("button", { name: "card.play — Video A" }); + const phoneHidden = (el: Element) => + el.classList.contains("hidden") && el.classList.contains("sm:table-cell"); + const heads = [...document.querySelectorAll("thead th")]; + const cells = [...document.querySelectorAll("tbody tr:first-child td")]; + // Columns: grip, #, title, channel, length, "…". + expect(heads.map(phoneHidden)).toEqual([false, true, false, true, false, false]); + expect(cells.map(phoneHidden)).toEqual([false, true, false, true, false, false]); + }); +}); + describe("Playlists — the rows are a keyboard list", () => { it("walks the rows with j and plays the focused one on Enter", async () => { mount({ 1: [vid("A", 0), vid("B", 0)] }); diff --git a/frontend/src/components/Playlists.tsx b/frontend/src/components/Playlists.tsx index 64509f72..04095caa 100644 --- a/frontend/src/components/Playlists.tsx +++ b/frontend/src/components/Playlists.tsx @@ -24,15 +24,10 @@ import { ArrowUp, ArrowUpToLine, Check, - ExternalLink, FolderInput, History, ListPlus, - Pencil, Play, - RotateCcw, - Trash2, - Upload, X, Youtube, } from "lucide-react"; @@ -47,19 +42,16 @@ import { formatRuntime } from "../lib/format"; import { applyView, sameOrder } from "../lib/playlistView"; const PlayerModal = lazy(() => import("./PlayerModal")); import { Button } from "./ui/Button"; -import { IconButton } from "./ui/IconButton"; import { LoadingState, StateMessage } from "./ui/QueryState"; import { type ActionMenuItem } from "./ui/ActionMenu"; -import { useComfortPin, pinVideo } from "./comfortPin"; +import { useComfortPin, pinPlaylist, pinVideo } from "./comfortPin"; import { ContextMenu } from "./ui/ContextMenu"; import { useContextMenu } from "./ui/useContextMenu"; import UndoToolbar from "./ui/UndoToolbar"; import NavGrid from "./ui/NavGrid"; import { PageToolbar } from "./ui/PageShell"; -import { useConfirm } from "./confirmContext"; import { usePlaylistActions } from "./usePlaylistActions"; import { usePlaylists } from "./playlistsContext"; -import { useMe } from "../lib/useMe"; import { usePref } from "../lib/usePref"; import { navItemAttrs } from "../lib/listNav"; import { ColumnHead } from "./ui/tableHeader"; @@ -71,7 +63,8 @@ import { DragHandle } from "./ui/DragHandle"; import { Input } from "./ui/Input"; import { CheckRow } from "./ui/NativeControls"; import { useReturnFocus } from "./ui/useReturnFocus"; -import { IconLink } from "./ui/LinkButton"; +import { MoreButton } from "./ui/MoreButton"; +import { useCopyItems } from "./copyItems"; // The playlist detail is a HYBRID table (E4 S4): real sortable/filterable column headers over a // body that is still a drag-reorderable @dnd-kit list. @@ -134,7 +127,8 @@ function Row({ }} // transition-COLORS, not `transition` — the bare utility animates transform too, which is // the same lag by another route (150ms behind the pointer, on every row). - className="border-b border-border/50 state-layer transition-colors" + // `group`: the row's "…" reveals on its hover or focus (`.reveal`), and stays on touch. + className="group border-b border-border/50 state-layer transition-colors" {...navItemAttrs(index)} {...menuProps} > @@ -142,7 +136,7 @@ function Row({ {col.key === GRIP_KEY ? ( // The one cell the column defs can't render: the handle needs this row's sortable @@ -164,14 +158,11 @@ function Row({ } export default function Playlists() { - const me = useMe(); - const canWrite = me.can_write; // The selected playlist lives in PlaylistsProvider, shared with the PlaylistsRail (which owns the // list + its rail controls). This detail pane reacts to the selection. const { selectedId } = usePlaylists(); const { t } = useTranslation(); const qc = useQueryClient(); - const confirm = useConfirm(); const plActions = usePlaylistActions(); const [renaming, setRenaming] = useState(false); const renameFocus = useReturnFocus(renaming); @@ -267,6 +258,10 @@ export default function Playlists() { // The comfort-shelf pin composes into the row's existing menu (E24 S3) rather than mounting a // second one on the same . const comfortPin = useComfortPin(rowMenu.open); + const copy = useCopyItems(); + // The header's "…": the list's own menu, the one its rail row opens (usePlaylistActions.items). + const headerMenu = useContextMenu(); + const headerPin = useComfortPin(headerMenu.open); const listQuery = useQuery({ queryKey: qk.playlists(), @@ -323,17 +318,15 @@ export default function Playlists() { const builtin = detail?.kind === "watch_later"; // YouTube-mirrored playlists can be edited locally and synced back (the read-sync skips a - // dirty mirror so it won't clobber unpushed edits); the YT-link button marks their origin. + // dirty mirror so it won't clobber unpushed edits); the header's "Unsynced" says when they differ. const editable = !builtin; const linked = editable && !!detail?.yt_playlist_id; - // LOCAL editing (reorder, remove) needs no YouTube write scope — `canWrite` gates only the - // push/delete-on-YouTube paths. Folding it in here would have taken drag and remove away from + // LOCAL editing (reorder, remove) needs no YouTube write scope — `can_write` gates only the + // push/delete-on-YouTube paths (usePlaylistActions). Folding it in here would have taken drag and remove away from // every account without the scope, which they have always had. const canEdit = editable; const canEditRef = useRef(canEdit); canEditRef.current = canEdit; - const [pushing, setPushing] = useState(false); - const [reverting, setReverting] = useState(false); // Stable (the memoized row callbacks depend on it), so it reads `shown` through the ref rather // than closing over it. @@ -350,11 +343,8 @@ export default function Playlists() { }); }, []); const onPlay = useCallback((v: Video) => openPlayer(v.id), [openPlayer]); - const onRemove = useCallback((v: Video) => { - void removeRef.current(v); - }, []); // 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 + // Delete key use — one optimistic update, one undo toast, 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. It removes from the list the player was // launched from (see `playing.listId`), not from whatever is selected by now. @@ -376,9 +366,25 @@ export default function Playlists() { [items], ); + // The row's "…" opens the row menu under itself (E28 C1). Which row's menu is open is part of the + // cell (aria-expanded), so the columns rebuild when it changes — a cheap array of closures. + const { buttonProps: rowMenuButton, open: rowMenuOpen, target: rowMenuTarget } = rowMenu; const columns = useMemo( - () => playlistColumns({ t, indexOf: storedIndex, onPlay, onRemove, canEdit }), - [t, storedIndex, onPlay, onRemove, canEdit], + () => + playlistColumns({ + t, + indexOf: storedIndex, + onPlay, + more: (v) => ( + + ), + }), + [t, storedIndex, onPlay, rowMenuButton, rowMenuOpen, rowMenuTarget], ); const filterActive = Object.values(filters).some(isActive); @@ -437,108 +443,6 @@ export default function Playlists() { qc.invalidateQueries({ queryKey: qk.playlist(listId) }); } - async function revertToYoutube() { - if (selectedId == null || !detail || reverting) return; - const ok = await confirm({ - title: t("playlists.revertTitle"), - message: t("playlists.revertMsg"), - confirmLabel: t("playlists.revertConfirm"), - danger: true, - }); - if (!ok) return; - setReverting(true); - try { - await api.revertPlaylist(selectedId); - refreshAll(); - notify({ level: "success", message: t("playlists.revertDone", { name: plName(detail) }) }); - } catch (e) { - notifyYouTubeActionError(e, t("playlists.revertFailed")); - } finally { - setReverting(false); - } - } - - async function pushToYoutube() { - if (selectedId == null || !detail || pushing) return; - setPushing(true); - try { - const plan = await api.playlistPushPlan(selectedId); - const nothingToDo = - plan.action === "update" && - !plan.add_yt && - !plan.add_local && - !plan.remove_yt && - !plan.remove_local && - !plan.to_reorder && - !plan.rename; - if (nothingToDo) { - notify({ level: "info", message: t("playlists.pushUpToDate", { name: plName(detail) }) }); - return; - } - if (!plan.affordable) { - notify({ - level: "warning", - message: t("playlists.pushNoQuota", { - left: plan.remaining_today, - units: plan.units_estimate, - }), - }); - return; - } - let message: string; - if (plan.action === "create") { - message = t("playlists.pushPlanCreate", { - count: plan.add_yt, - units: plan.units_estimate, - left: plan.remaining_today, - }); - } else { - // One sentence per side that actually changes; the untouched side gets a short "stays - // unchanged" reassurance instead of a "+0 / −0" the reader has to decode. - const here: string[] = []; - if (plan.add_local) here.push(t("playlists.mergeAddLocal", { count: plan.add_local })); - if (plan.remove_local) - here.push(t("playlists.mergeRemoveLocal", { count: plan.remove_local })); - const yt: string[] = []; - if (plan.add_yt) yt.push(t("playlists.mergeAddYt", { count: plan.add_yt })); - if (plan.remove_yt) yt.push(t("playlists.mergeRemoveYt", { count: plan.remove_yt })); - if (plan.to_reorder) yt.push(t("playlists.mergeReorder", { count: plan.to_reorder })); - if (plan.rename) yt.push(t("playlists.mergeRename")); - const sentences = [...here, ...yt]; - if (here.length && !yt.length) sentences.push(t("playlists.mergeYtUnchanged")); - if (yt.length && !here.length) sentences.push(t("playlists.mergeLocalUnchanged")); - const quota = - plan.units_estimate > 0 - ? t("playlists.mergeQuota", { - units: plan.units_estimate, - left: plan.remaining_today, - }) - : t("playlists.mergeQuotaFree"); - message = sentences.join(" ") + quota; - } - const ok = await confirm({ - title: t("playlists.pushTitle"), - message, - confirmLabel: t("playlists.pushConfirm"), - }); - if (!ok) return; - const r = await api.pushPlaylist(selectedId); - refreshAll(); - if (r.failures.length) { - notify({ - level: "warning", - message: t("playlists.pushPartial", { count: r.failures.length }), - }); - } else { - notify({ level: "success", message: t("playlists.pushDone", { name: plName(detail) }) }); - } - } catch (e) { - notifyYouTubeActionError(e, t("playlists.pushFailed")); - } finally { - setPushing(false); - } - } - function onDragEnd(e: DragEndEvent) { const { active: a, over } = e; if (!over || a.id === over.id || selectedId == null) return; @@ -548,26 +452,20 @@ export default function Playlists() { order.set(arrayMove(items, oldIndex, newIndex)); // onApply persists } - /** @returns whether the item was actually removed — the player needs to know, because it must + /** Takes a row out at once and says so in a toast whose Undo puts it back where it was (E28 C1: + * the X column became the Delete key + this toast; no confirm, because nothing is lost). + * @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, 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 }), - confirmLabel: t("playlists.removeItem"), - 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. + // be removing from the list it was launched from while another one is selected. 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. + // A removal isn't itself on the reorder undo stack, but it no longer THROWS AWAY that 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)}`; @@ -578,17 +476,19 @@ export default function Playlists() { // so the guard reset the order: the removed row came back and the undo history the rebase had // kept went with it. (A failure resyncs below, which puts the row back where it belongs.) const before = qc.getQueryData(qk.playlist(listId)); + // Where it stood in the STORED order — what the Undo puts it back at (the server's list, so a + // view sort on screen does not decide it). + const at = before?.items.findIndex((v) => v.id === video.id) ?? -1; qc.setQueryData(qk.playlist(listId), (old) => old ? { ...old, items: old.items.filter((v) => v.id !== video.id) } : old, ); try { await api.removeFromPlaylist(listId, video.id); refreshAll(listId); - return true; } catch (e) { - // Said 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) — unless `req` already - // put it in its error dialog, which `notifyActionError` checks. + // Said here, as moveItemTo does, or a removal fails in silence (inside the full-screen player + // there is not even a row snapping back to show it) — unless `req` already put it in its error + // dialog, which `notifyActionError` checks. notifyActionError(e, t("playlists.removeFailed")); // …and the row goes back NOW, not when the resync lands: a removal that failed because the // server was unreachable has a resync that fails too, and the list kept showing the row as @@ -597,6 +497,24 @@ export default function Playlists() { refreshAll(listId); return false; } + notify({ + level: "success", + message: t("playlists.removedToast", { title: video.title }), + action: { + label: t("common.undo"), + // ONE request that puts it back at its index (the server renumbers around it): an append + // plus a whole-list reorder would land it at the end whenever the second call failed. + onClick: () => + void api + .addToPlaylist(listId, video.id, at < 0 ? undefined : at) + .catch((e: unknown) => notifyActionError(e, t("playlists.undoRemoveFailed"))) + .finally(() => { + refreshAll(listId); + qc.invalidateQueries({ queryKey: qk.playlistMembership(video.id) }); + }), + }, + }); + return true; } // Kept in a ref so the memoized column callbacks don't rebuild on every state change. const removeRef = useRef(removeItem); @@ -659,13 +577,6 @@ export default function Playlists() { if (name && name !== detail?.name) await plActions.rename(selectedId, name); } - async function deletePlaylist() { - if (!detail) return; - // The dialog, the "delete on YouTube too?" branch and the re-selection live in - // usePlaylistActions — the rail's context menu deletes through the SAME code. - await plActions.remove(detail); - } - function playerState(id: string, status: string) { api .setState(id, status) @@ -729,6 +640,10 @@ export default function Playlists() { onSelect: () => openPlayer(ctxRow.id), }, ...comfortPin.item(pinVideo(ctxRow.id), ctxRow.title ?? ""), + ...copy(ctxRow.title, ctxRow.watch_url).map((it, i) => ({ + ...it, + separatorBefore: i === 0, + })), ...(canEdit ? ([ ...(recentMoveTarget @@ -802,6 +717,17 @@ export default function Playlists() { : []), ]; + const headerPl = headerMenu.target; + const headerItems: ActionMenuItem[] = !headerPl + ? [] + : plActions.items(headerPl, { + onRename: () => { + setRenameValue(headerPl.name); + setRenaming(true); + }, + pin: headerPin.item(pinPlaylist(headerPl.id), plName(headerPl)), + }); + // The open player, at ONE place in the tree whichever state the page below it is in. Selecting // another list under an open player (the command palette works over it) puts the page into // "Loading…" until that list arrives; with the player rendered only in the loaded branch, that @@ -894,32 +820,7 @@ export default function Playlists() { aria-label={t("playlists.rename")} /> ) : ( - <> -

{plName(detail)}

- {editable && ( - { - setRenameValue(detail.name); - setRenaming(true); - }} - label={t("playlists.rename")} - > - - - )} - {detail.yt_playlist_id && ( - - - - )} - +

{plName(detail)}

)}
{linked && detail.dirty && ( @@ -939,39 +840,15 @@ export default function Playlists() { > {t("playlists.playAll")} - {editable && canWrite && ( - - {linked ? : } - - )} - {linked && detail.dirty && ( - - - - )} - {editable && ( - - - - )} + {/* Rename, the pin, YouTube (open / push / revert) and Delete: the list's ONE + menu (E28 C1), the same the rail row opens — the header keeps Play all. */} +
@@ -1074,7 +951,18 @@ export default function Playlists() { {/* The rows are a keyboard list: `j`/`k` and the arrows walk them, Enter plays. The grip keeps Space/Enter, and during a keyboard drag the arrows too (listNav leaves a key dnd-kit has handled). */} - + { + const v = shownRef.current[i]; + if (v) void removeItem(v); + } + : undefined + } + > {shown.map((v, i) => ( + {player} diff --git a/frontend/src/components/PlaylistsRail.rename.dom.test.tsx b/frontend/src/components/PlaylistsRail.rename.dom.test.tsx index bed68b29..46b34c6c 100644 --- a/frontend/src/components/PlaylistsRail.rename.dom.test.tsx +++ b/frontend/src/components/PlaylistsRail.rename.dom.test.tsx @@ -23,10 +23,13 @@ vi.mock("./comfortPin", () => ({ useComfortPin: () => ({ item: () => [] }), pinPlaylist: (id: number) => id, })); -const rename = vi.fn(async () => {}); -vi.mock("./usePlaylistActions", () => ({ - usePlaylistActions: () => ({ rename, remove: vi.fn() }), +// The real usePlaylistActions builds the menu (the rename row is its first); only the dialogs are +// stubbed, and the write is watched at the API. +vi.mock("./confirmContext", () => ({ + useChoice: () => async () => null, + useConfirm: () => async () => false, })); +const rename = vi.fn(async () => ({}) as never); // The SidePanel/PanelGroups chrome is not what is under test: render the rail's bodies directly. vi.mock("./FilterRail", () => ({ default: ({ items }: { items: { id: string; body: ReactNode }[] }) => ( @@ -74,6 +77,7 @@ const alpha = { async function openRename() { vi.spyOn(api, "playlists").mockResolvedValue([alpha]); + vi.spyOn(api, "renamePlaylist").mockImplementation(rename); const qc = new QueryClient({ defaultOptions: { queries: { retry: false } } }); render( @@ -117,3 +121,40 @@ describe("PlaylistsRail · rename from the keyboard", () => { await vi.waitFor(() => expect(document.activeElement).toBe(rowButton())); }); }); + +// E28 C1: the row's "…" opens the same menu as its right-click — the list's one action list. +describe("PlaylistsRail · the row's '…'", () => { + it("opens the playlist's menu under the row, and says so", async () => { + vi.spyOn(api, "playlists").mockResolvedValue([alpha]); + const qc = new QueryClient({ defaultOptions: { queries: { retry: false } } }); + render( + + + {}} + collapsed={false} + onToggleCollapse={() => {}} + /> + + , + ); + await screen.findByText("alpha"); + const more = screen.getByRole("button", { name: "playlists.railMenu" }); + expect(more.getAttribute("aria-expanded")).toBe("false"); + fireEvent.click(more); + await vi.waitFor(() => + expect(screen.getAllByRole("menuitem", { hidden: true }).map((b) => b.textContent)).toEqual([ + "playlists.rename", + "playlists.exportToYoutube", + "playlists.delete", + ]), + ); + expect( + screen.getByRole("button", { name: "playlists.railMenu" }).getAttribute("aria-expanded"), + ).toBe("true"); + // It is not inside the row's own button: a button cannot hold a button. + expect(more.closest("button")).toBe(more); + expect(rowButton()?.contains(more)).toBe(false); + }); +}); diff --git a/frontend/src/components/PlaylistsRail.tsx b/frontend/src/components/PlaylistsRail.tsx index ed9616e7..ef604e63 100644 --- a/frontend/src/components/PlaylistsRail.tsx +++ b/frontend/src/components/PlaylistsRail.tsx @@ -2,7 +2,7 @@ import { useCallback, useEffect, useMemo, useRef, useState } from "react"; import { useTranslation } from "react-i18next"; import { useMutation, useQuery, useQueryClient } from "@tanstack/react-query"; import { qk } from "../lib/queryKeys"; -import { ListVideo, Pencil, Pin, Plus, RefreshCw, Trash2, Youtube } from "lucide-react"; +import { ListVideo, Pin, Plus, RefreshCw, Youtube } from "lucide-react"; import { api, type Playlist } from "../lib/api"; import { formatRuntime } from "../lib/format"; import { notify } from "../lib/notifications"; @@ -26,6 +26,7 @@ import PlaylistLimitsHint from "./PlaylistLimitsHint"; import { IconButton } from "./ui/IconButton"; import { Pressable } from "./ui/Pressable"; import { Input } from "./ui/Input"; +import { MoreButton } from "./ui/MoreButton"; type PlSort = { key: "custom" | "name" | "count" | "duration" | "recent"; @@ -199,43 +200,17 @@ export default function PlaylistsRail({ } const ctxPl = railMenu.target; - // The built-in Watch later list is renameable/deletable nowhere else in the UI either; the rows - // stay visible and disabled so the menu doesn't change shape from list to list. - const ctxBuiltin = ctxPl?.kind === "watch_later"; - // Interpolated, not spelled out: the HU copy said "Watch later", a name the Hungarian UI never - // shows for this list ("Megnézem később") — so the message named a list the reader had never seen - // (E10 review round 1). Passing the localized name makes the two impossible to drift. - const lockedNote = ctxBuiltin - ? t("playlists.builtinLocked", { name: t("playlists.watchLater") }) - : undefined; + // The list's ONE menu (usePlaylistActions.items), the same the page header's "…" opens; the rail + // renames IN the row that was right-clicked — the action lands where the gesture did. const railItems: ActionMenuItem[] = !ctxPl ? [] - : [ - { - id: "rename", - label: t("playlists.rename"), - icon: , - disabled: ctxBuiltin, - note: lockedNote, - onSelect: () => { - setRenameValue(ctxPl.name); - setRenamingId(ctxPl.id); - }, + : plActions.items(ctxPl, { + onRename: () => { + setRenameValue(ctxPl.name); + setRenamingId(ctxPl.id); }, - ...comfortPin.item(pinPlaylist(ctxPl.id), plName(ctxPl)), - { - id: "delete", - label: t("playlists.delete"), - icon: , - separatorBefore: true, - danger: true, - disabled: ctxBuiltin, - note: lockedNote, - // The same dialog (and the same "what gets selected next" rule) as the detail header's - // trash button — one implementation, two entry points. - onSelect: () => void plActions.remove(ctxPl), - }, - ]; + pin: comfortPin.item(pinPlaylist(ctxPl.id), plName(ctxPl)), + }); const items: PanelItem[] = [ { @@ -350,44 +325,59 @@ export default function PlaylistsRail({ /> ) : ( - { - if (pl.id === selectedId) selectedRef.current = el; - // Back from the rename field by Enter/Escape — see refocusRowRef. - if (el && refocusRowRef.current === pl.id) { - refocusRowRef.current = null; - el.focus(); - } - }} - onClick={() => setSelectedId(pl.id)} - {...railMenu.triggerProps(pl)} - layer={pl.id !== selectedId} - className={`${ROW_BASE} ${ - pl.id === selectedId ? "bg-accent/15 border-accent" : "border-transparent" - }`} - > -
- {pl.cover_thumbnail && ( - - )} -
-
-
- {plName(pl)} - {(pl.source === "youtube" || pl.yt_playlist_id) && ( - + { + if (pl.id === selectedId) selectedRef.current = el; + // Back from the rename field by Enter/Escape — see refocusRowRef. + if (el && refocusRowRef.current === pl.id) { + refocusRowRef.current = null; + el.focus(); + } + }} + onClick={() => setSelectedId(pl.id)} + layer={pl.id !== selectedId} + // pr-9: the "…" sits over the row's right end, so the name truncates short of it. + className={`${ROW_BASE} pr-9 ${ + pl.id === selectedId ? "bg-accent/15 border-accent" : "border-transparent" + }`} + > +
+ {pl.cover_thumbnail && ( + )}
-
- {t("playlists.itemCount", { count: pl.item_count })} - {pl.total_duration_seconds > 0 && - ` · ${formatRuntime(pl.total_duration_seconds)}`} +
+
+ {plName(pl)} + {(pl.source === "youtube" || pl.yt_playlist_id) && ( + + )} +
+
+ {t("playlists.itemCount", { count: pl.item_count })} + {pl.total_duration_seconds > 0 && + ` · ${formatRuntime(pl.total_duration_seconds)}`} +
-
- + + +
), )}
diff --git a/frontend/src/components/playlistColumns.tsx b/frontend/src/components/playlistColumns.tsx index 8445f3c0..67e01875 100644 --- a/frontend/src/components/playlistColumns.tsx +++ b/frontend/src/components/playlistColumns.tsx @@ -1,9 +1,9 @@ -import { Play, X } from "lucide-react"; +import type { ReactNode } from "react"; +import { Play } from "lucide-react"; import type { TFunction } from "i18next"; import type { Video } from "../lib/api"; import { formatDuration } from "../lib/format"; import type { Column } from "../lib/tableColumns"; -import { IconButton } from "./ui/IconButton"; import { Pressable } from "./ui/Pressable"; // Column definitions for the playlist detail table, in their own file like `channelColumns.tsx`. @@ -18,16 +18,15 @@ export function playlistColumns({ t, indexOf, onPlay, - onRemove, - canEdit, + more, }: { t: TFunction; /** Position in the STORED order (1-based on screen) — not the row's position in the current view, * so a view-sort never renumbers the playlist under the reader. */ indexOf: (v: Video) => number; onPlay: (v: Video) => void; - onRemove: (v: Video) => void; - canEdit: boolean; + /** The row's "…" — the visible door to its menu (E28 C1), where "Remove" now lives. */ + more: (v: Video) => ReactNode; }): Column