diff --git a/backend/app/reader/service.py b/backend/app/reader/service.py index 93deffb9..bae0d881 100644 --- a/backend/app/reader/service.py +++ b/backend/app/reader/service.py @@ -292,7 +292,7 @@ def set_progress(db: Session, user: User, video_id: str, position: int) -> None: # Every reason a move is refused; the UI names each one (`playlists.moveErrors.`, held by the # frontend's backendIds contract, which reads this Literal). -MoveReason = Literal["same_playlist", "not_in_playlist"] +MoveReason = Literal["same_playlist"] class MoveRefused(Exception): @@ -447,41 +447,6 @@ def delete_playlist(db: Session, pl: Playlist) -> None: db.flush() -def move_item(db: Session, src: Playlist, dst: Playlist, video_id: str) -> None: - """Move one video from `src` into `dst`, in the caller's one transaction. - - The client-side shape — add to the target, then remove from the source — leaves the video in - BOTH lists whenever the second call fails. Idempotent: a video already in the target is simply - dropped from the source (moving into a list that has it is a removal, not a duplicate). Raises - `MoveRefused` for the same list twice, or a video a concurrent edit already took out.""" - if src.id == dst.id: - raise MoveRefused("same_playlist") - # Both lists, in id order: a move A→B beside a move B→A would otherwise lock crosswise. - lock_playlists(db, src.id, dst.id) - row = queries.item(db, src.id, video_id) - if row is None: - raise MoveRefused("not_in_playlist") - dst_changed = False - if queries.item(db, dst.id, video_id) is None: - # Re-parent the existing row rather than delete+insert: `added_at` ("when I saved this") - # survives the move, and there is no window where the video belongs to neither list. The - # check above holds: both lists are locked, and an insert into the target (another tab's - # add, a YouTube merge) waits for the lock — its foreign key needs the target row. - row.playlist_id = dst.id - row.position = next_item_position(db, dst.id) - dst_changed = True - if not dst_changed: - db.delete(row) - # The source always changed; the target only when the row landed in it. Touching a target - # that already had the video promoted an untouched list in the rail's "recently changed" sort. - recompute_dirty(db, src) - touch(src) - if dst_changed: - recompute_dirty(db, dst) - touch(dst) - db.flush() - - def remove_items(db: Session, pl: Playlist, video_ids: list[str]) -> int: """Remove these videos from a playlist, in one transaction; a video not in it any more is simply not counted. Says how many went.""" @@ -534,12 +499,14 @@ def restore_items(db: Session, pl: Playlist, video_ids: list[str], positions: li def move_items( db: Session, src: Playlist, dst: Playlist, video_ids: list[str] ) -> tuple[list[str], list[str]]: - """Move these videos from `src` into `dst`, in the source's order whatever order they were picked - in, as `move_item` does one. Returns the ones that landed in the target and the ones it already - held, which only left the source: the Undo needs to tell them apart. A video a concurrent edit - already took out of the source is in neither.""" + """Move these videos from `src` into `dst`, in the caller's one transaction, in the source's + order whatever order they were picked in. Returns the ones that landed in the target and the ones + it already held, which only left the source: the Undo needs to tell them apart. A video a + concurrent edit already took out of the source is in neither. Raises `MoveRefused` for the same + list twice.""" if src.id == dst.id: raise MoveRefused("same_playlist") + # Both lists, in id order: a move A→B beside a move B→A would otherwise lock crosswise. lock_playlists(db, src.id, dst.id) wanted = set(video_ids) rows = [it for it in queries.items(db, src.id) if it.video_id in wanted] @@ -553,13 +520,18 @@ def move_items( db.delete(row) else: moved.append(row.video_id) - # Re-parented, as `move_item` says why: `added_at` survives the move. + # Re-parented rather than deleted and inserted: `added_at` ("when I saved this") survives + # the move, and there is no window where the video belongs to neither list. `held` stays + # true: both lists are locked, and an insert into the target (another tab's add, a + # YouTube merge) waits for the lock, as its foreign key needs the target row. row.playlist_id = dst.id row.position = position position += 1 if rows: recompute_dirty(db, src) touch(src) + # Only when a row landed in it: touching a target that already had the video promoted an untouched + # list in the rail's "recently changed" sort. if moved: recompute_dirty(db, dst) touch(dst) diff --git a/backend/app/routes/playlists.py b/backend/app/routes/playlists.py index a4ca9d41..52e8ec47 100644 --- a/backend/app/routes/playlists.py +++ b/backend/app/routes/playlists.py @@ -421,44 +421,6 @@ def remove_item( return {"ok": True, "playlist_id": pl.id, "video_id": video_id} -class MoveItemIn(BaseModel): - target_playlist_id: int - - -@router.post("/{playlist_id}/items/{video_id}/move") -def move_item( - playlist_id: int, - video_id: str, - body: MoveItemIn, - user: User = Depends(current_user), - db: Session = Depends(get_db), -) -> dict: - """Move one video from a playlist into another of the caller's playlists, atomically. - - ONE transaction on purpose. The obvious client-side shape — add to the target, then remove - from the source — leaves the video in BOTH lists whenever the second call fails, which is - exactly the kind of half-applied state this codebase treats as a bug rather than an edge - case. Idempotent: if the video is already in the target, the source row is simply dropped - (moving into a list that already has it is a removal, not a duplicate).""" - src = _own_playlist(db, user, playlist_id) - # Its own reason: "this playlist doesn't exist any more" about a TARGET that is gone was read as - # the list on screen — which is still there. - dst = queries.own_playlist(db, user, body.target_playlist_id) - if dst is None: - raise not_found("move_target") - try: - service.move_item(db, src, dst, video_id) - except service.MoveRefused as e: - # A same-list move is a bad request; a video a concurrent edit (another tab, a YouTube - # re-pull) already took out is gone. - status = 400 if e.reason == "same_playlist" else 404 - raise HTTPException( - status_code=status, detail={"code": "playlist_move", "reason": e.reason} - ) from None - db.commit() - return {"ok": True, "from_playlist_id": src.id, "to_playlist_id": dst.id, "video_id": video_id} - - class VideoIdsIn(BaseModel): video_ids: list[str] = [] @@ -542,8 +504,13 @@ def move_items( user: User = Depends(current_user), db: Session = Depends(get_db), ) -> dict: - """Move many videos into another of the caller's playlists, in one transaction, for the reason - `move_item` gives. Says which landed there and which it already held, for the "Undo".""" + """Move videos into another of the caller's playlists, atomically. + + ONE transaction on purpose. The obvious client-side shape — add to the target, then remove + from the source — leaves a video in BOTH lists whenever the second call fails. A video the + target already has only leaves the source, and one a concurrent edit (another tab, a YouTube + re-pull) already took out is skipped. Says which landed there and which it already held, for + the "Undo".""" src = _own_playlist(db, user, playlist_id) dst = _other_list(db, user, body.target_playlist_id) try: diff --git a/backend/tests/test_playlist_move.py b/backend/tests/test_playlist_move.py index d0b74c58..c430f19e 100644 --- a/backend/tests/test_playlist_move.py +++ b/backend/tests/test_playlist_move.py @@ -51,7 +51,8 @@ def _ids(db, pl_id): def _move(client, src, video_id, dst): return client.post( - f"/api/playlists/{src}/items/{video_id}/move", json={"target_playlist_id": dst} + f"/api/playlists/{src}/items/move-bulk", + json={"video_ids": [video_id], "target_playlist_id": dst}, ) @@ -67,7 +68,8 @@ def test_move_leaves_the_video_in_exactly_one_list(db, make_user, as_user): "ok": True, "from_playlist_id": src.id, "to_playlist_id": dst.id, - "video_id": "v1", + "moved": ["v1"], + "already_there": [], } db.expire_all() assert _ids(db, src.id) == ["v2"] @@ -116,8 +118,9 @@ def test_move_of_a_video_that_left_the_source_reports_it(db, make_user, as_user) dst = _playlist(db, user, "Target", []) r = _move(as_user(user), src.id, "v1", dst.id) - assert r.status_code == 404 - assert r.json()["detail"] == {"code": "playlist_move", "reason": "not_in_playlist"} + assert r.status_code == 200 + assert (r.json()["moved"], r.json()["already_there"]) == ([], []) + assert _ids(db, dst.id) == [] def test_move_into_someone_elses_list_404s_and_changes_nothing(db, make_user, as_user): diff --git a/backend/tests/test_playlist_position_lock.py b/backend/tests/test_playlist_position_lock.py index 308c1b7d..bd632ef7 100644 --- a/backend/tests/test_playlist_position_lock.py +++ b/backend/tests/test_playlist_position_lock.py @@ -107,7 +107,8 @@ PLEX = PlexPlaylist.__table__.name ), lambda c, pid, other: c.post("/api/playlists/watch-later", json={"video_id": "v2"}), lambda c, pid, other: c.post( - f"/api/playlists/{pid}/items/v1/move", json={"target_playlist_id": other} + f"/api/playlists/{pid}/items/move-bulk", + json={"video_ids": ["v1"], "target_playlist_id": other}, ), lambda c, pid, other: c.put(f"/api/playlists/{pid}/order", json={"video_ids": ["v1"]}), ], @@ -173,7 +174,8 @@ PLEX_ITEMS = PlexPlaylistItem.__table__.name ), lambda c, pid, other: c.delete(f"/api/playlists/{pid}/items/v1"), lambda c, pid, other: c.post( - f"/api/playlists/{pid}/items/v1/move", json={"target_playlist_id": other} + f"/api/playlists/{pid}/items/move-bulk", + json={"video_ids": ["v1"], "target_playlist_id": other}, ), lambda c, pid, other: c.put(f"/api/playlists/{pid}/order", json={"video_ids": ["v1"]}), # the ORM cascade deletes the items before the list row @@ -199,7 +201,8 @@ def test_a_move_locks_both_lists_in_one_ordered_statement(db, make_user, as_user pid, other = _yt(db, user) statements.clear() r = as_user(user).post( - f"/api/playlists/{pid}/items/v1/move", json={"target_playlist_id": other} + f"/api/playlists/{pid}/items/move-bulk", + json={"video_ids": ["v1"], "target_playlist_id": other}, ) assert r.status_code == 200, r.text lock = _locked_before_items(statements, YT, YT_ITEMS) diff --git a/backend/tests/test_reader_service.py b/backend/tests/test_reader_service.py index 63e4c0b8..ac512f6c 100644 --- a/backend/tests/test_reader_service.py +++ b/backend/tests/test_reader_service.py @@ -235,14 +235,13 @@ def test_a_move_never_leaves_a_video_in_two_lists(db, make_user): service.add_item(db, dst, "b", mark_dirty=False) db.commit() with pytest.raises(service.MoveRefused, match="same_playlist"): - service.move_item(db, src, src, "a") - service.move_item(db, src, dst, "a") - service.move_item(db, src, dst, "b") # already in the target: dropped from the source + service.move_items(db, src, src, ["a"]) + assert service.move_items(db, src, dst, ["a"]) == (["a"], []) + # already in the target: dropped from the source + assert service.move_items(db, src, dst, ["b"]) == ([], ["b"]) db.commit() assert (_ids(db, src), _ids(db, dst)) == ([], ["b", "a"]) - with pytest.raises(service.MoveRefused, match="not_in_playlist"): - service.move_item(db, src, dst, "a") - db.rollback() + assert service.move_items(db, src, dst, ["a"]) == ([], []) def test_a_reorder_keeps_what_it_left_out_after_the_listed(db, make_user): diff --git a/docs/DESIGN.md b/docs/DESIGN.md index 8080c1f2..64b062ef 100644 --- a/docs/DESIGN.md +++ b/docs/DESIGN.md @@ -540,7 +540,10 @@ of a full-width panel and nothing on the right. over the column for the rows shown. Shift+click selects a range, `x` toggles the row under the keyboard, Esc clears. A selected row wears the accent tint as well as its tick. An action covers the selected rows shown; a row a filter hides stays selected and comes back ticked; another list starts over. The row's own - menu still acts on that row alone. + menu still acts on that row alone. While anything is selected the toolbar's second row is the action band: + the count (and how many a filter hides), the last move's target as one click, Move to, Remove, Clear + selection. An action clears the selection, undoes by its toast and, when it fails, ticks its rows again. + Delete takes the selection when there is one, else the row under the keyboard, with no confirm. - **Motion:** 150–300ms micro-interactions, transform/opacity only; `prefers-reduced-motion` is respected. [WCAG 2.3.3] @@ -840,3 +843,5 @@ of a full-width panel and nothing on the right. | 2026-11-01 | Clear watch progress (owner E a) | A video menu's Reset reads "Clear watch progress" in English (HU "Megtekintés visszaállítása" stays); "Reset view" is only a page's view reset (sort, filters, grouping) | "Reset view" for both; the playlist page's view reset renamed instead | On a playlist page the two stood one right-click apart under one name, and one of them erases the video's progress with no undo | | 2026-11-01 | Clear watch progress clears only that (owner A) | The row shows only on a video with a resume point and clears that alone: a hidden video stays hidden, a watched one watched. Un-hiding and un-watching have rows of their own | | 2026-11-01 | Selecting a list's rows (owner D2 a) | An always visible box first in the row and one over the column for the rows shown; Shift+click a range, `x` the row under the keyboard, Esc clears; the selected row wears `accent/12` with its tick. Rows a filter hides stay selected, an action covers those shown; Watch later offers no selecting. | +| 2026-11-01 | Selection band (owner D1 a, D3 a, D4 a) | The toolbar's second row while rows are selected, no floating bar: move (the last target as one click) and remove, no copy; both undo back to the places, a video the target held before stays there; a single row's move undoes too. | +| 2026-11-01 | Grip on a phone (owner A) | Below a 24rem table a playlist row has no grip: the title keeps its width, and the row menu keeps the moves. | diff --git a/frontend/src/components/ui/RowSelectBox.tsx b/frontend/src/components/ui/RowSelectBox.tsx index 86c91b78..f9bed885 100644 --- a/frontend/src/components/ui/RowSelectBox.tsx +++ b/frontend/src/components/ui/RowSelectBox.tsx @@ -1,3 +1,4 @@ +import type { RefObject } from "react"; import { Checkbox } from "./NativeControls"; /** A table row's selecting box, and the same box over the column for all the rows. The whole cell is @@ -9,7 +10,9 @@ export function RowSelectBox({ mixed, title, onToggle, + ref, }: { + ref?: RefObject; label: string; checked: boolean; mixed?: boolean; @@ -26,6 +29,7 @@ export function RowSelectBox({ className="-ml-2 grid min-h-6 w-9 cursor-pointer place-items-center touch-target" > { it("go through features/video/videoWrites, never the api directly", () => { @@ -38,7 +38,8 @@ describe("a video's state, Watch-later and playlist-item writes", () => { it("catches a write named without being called on the spot, and a playlist item's", () => { expect(WRITES.test("(saved ? api.watchLaterAdd : api.watchLaterRemove)(id)")).toBe(true); expect(WRITES.test("await api\n .removeFromPlaylist(listId, id)")).toBe(true); - expect(WRITES.test("api.moveToPlaylist(a, id, b)")).toBe(true); + expect(WRITES.test("api.moveManyToPlaylist(a, ids, b)")).toBe(true); + expect(WRITES.test("api.undoPlaylistMove(a, b, m, t, p)")).toBe(true); expect(WRITES.test("this.setState(next); api.playlists()")).toBe(false); }); }); diff --git a/frontend/src/features/video/videoWrites.dom.test.ts b/frontend/src/features/video/videoWrites.dom.test.ts index aa10f1ef..5b1b8e39 100644 --- a/frontend/src/features/video/videoWrites.dom.test.ts +++ b/frontend/src/features/video/videoWrites.dom.test.ts @@ -9,7 +9,10 @@ import { api } from "../../lib/api"; import { qk } from "../../lib/queryKeys"; import { addPlaylistItem, - movePlaylistItem, + movePlaylistItems, + removePlaylistItems, + restorePlaylistItems, + undoPlaylistMove, removePlaylistItem, clearVideoProgress, writeVideoState, @@ -77,12 +80,44 @@ describe("videoWrites", () => { expect(marks(qc)).toBeUndefined(); }); - it("marks a move into and out of the Watch later list", async () => { - vi.spyOn(api, "moveToPlaylist").mockResolvedValue({ ok: true } as never); + it("marks a move into and out of the Watch later list, by what the server moved", async () => { + vi.spyOn(api, "moveManyToPlaylist").mockResolvedValue({ + ok: true, + moved: ["X"], + already_there: ["Y"], + } as never); const qc = new QueryClient(); - await movePlaylistItem(qc, MINE, "X", WATCH_LATER); + await movePlaylistItems(qc, MINE, ["X", "Y", "gone"], WATCH_LATER); + expect(marks(qc)).toEqual({ X: { saved: true }, Y: { saved: true } }); + await movePlaylistItems(qc, WATCH_LATER, ["X", "Y", "gone"], MINE); + expect(marks(qc)).toEqual({ X: { saved: false }, Y: { saved: false } }); + }); + + it("marks a bulk removal from Watch later, and its restore", async () => { + vi.spyOn(api, "removeManyFromPlaylist").mockResolvedValue({ ok: true } as never); + vi.spyOn(api, "restoreToPlaylist").mockResolvedValue({ ok: true } as never); + const qc = new QueryClient(); + await removePlaylistItems(qc, WATCH_LATER, ["X"]); + expect(marks(qc)).toEqual({ X: { saved: false } }); + await restorePlaylistItems(qc, WATCH_LATER, ["X"], [0]); expect(marks(qc)).toEqual({ X: { saved: true } }); - await movePlaylistItem(qc, WATCH_LATER, "X", MINE); + }); + + it("takes a move into Watch later back: only what landed there loses the mark", async () => { + vi.spyOn(api, "undoPlaylistMove").mockResolvedValue({ ok: true } as never); + const qc = new QueryClient(); + await undoPlaylistMove(qc, MINE, WATCH_LATER, { + moved: ["X"], + already_there: ["Y"], + positions: [0, 1], + }); + expect(api.undoPlaylistMove).toHaveBeenCalledWith( + MINE.id, + WATCH_LATER.id, + ["X"], + ["Y"], + [0, 1], + ); expect(marks(qc)).toEqual({ X: { saved: false } }); }); diff --git a/frontend/src/features/video/videoWrites.ts b/frontend/src/features/video/videoWrites.ts index 4f928095..09c4f398 100644 --- a/frontend/src/features/video/videoWrites.ts +++ b/frontend/src/features/video/videoWrites.ts @@ -59,11 +59,52 @@ export function removePlaylistItem(qc: QueryClient, list: ListRef, id: string) { }); } -/** Move a video from one playlist to another, in one request (see `api.moveToPlaylist`). */ -export function movePlaylistItem(qc: QueryClient, from: ListRef, id: string, to: ListRef) { - return api.moveToPlaylist(from.id, id, to.id).then((r) => { - markIfWatchLater(qc, from, id, false); - markIfWatchLater(qc, to, id, true); +/** Take videos out of a playlist, in one request. */ +export function removePlaylistItems(qc: QueryClient, list: ListRef, ids: string[]) { + return api.removeManyFromPlaylist(list.id, ids).then((r) => { + for (const id of ids) markIfWatchLater(qc, list, id, false); return r; }); } + +/** Put removed videos back at their places, in one request. */ +export function restorePlaylistItems( + qc: QueryClient, + list: ListRef, + ids: string[], + positions: number[], +) { + return api.restoreToPlaylist(list.id, ids, positions).then((r) => { + for (const id of ids) markIfWatchLater(qc, list, id, true); + return r; + }); +} + +/** Move videos from one playlist to another, in one request (see `api.moveManyToPlaylist`). Marks + * what the server says it moved: a video a concurrent edit already took out is in neither answer. */ +export function movePlaylistItems(qc: QueryClient, from: ListRef, ids: string[], to: ListRef) { + return api.moveManyToPlaylist(from.id, ids, to.id).then((r) => { + for (const id of [...r.moved, ...r.already_there]) { + markIfWatchLater(qc, from, id, false); + markIfWatchLater(qc, to, id, true); + } + return r; + }); +} + +/** Take a move back: every video returns to `from`, and only the ones that landed in `to` leave it; + * it held the others before the move. */ +export function undoPlaylistMove( + qc: QueryClient, + from: ListRef, + to: ListRef, + back: { moved: string[]; already_there: string[]; positions: number[] }, +) { + return api + .undoPlaylistMove(from.id, to.id, back.moved, back.already_there, back.positions) + .then((r) => { + for (const id of back.moved) markIfWatchLater(qc, to, id, false); + for (const id of [...back.moved, ...back.already_there]) markIfWatchLater(qc, from, id, true); + return r; + }); +} diff --git a/frontend/src/i18n/locales/en/keys.json b/frontend/src/i18n/locales/en/keys.json index cd7eefab..c4932eb1 100644 --- a/frontend/src/i18n/locales/en/keys.json +++ b/frontend/src/i18n/locales/en/keys.json @@ -35,7 +35,7 @@ "listStep": "Next / previous item", "gridMove": "Move around the list", "open": "Open the current item", - "listRemove": "Remove the current item from the list", + "listRemove": "Remove the selected rows, or else the current one", "listSelect": "Select or deselect the current item", "listClearSelection": "Clear the selection", "cap": { diff --git a/frontend/src/i18n/locales/en/playlists.json b/frontend/src/i18n/locales/en/playlists.json index 1f14fd65..362ebed9 100644 --- a/frontend/src/i18n/locales/en/playlists.json +++ b/frontend/src/i18n/locales/en/playlists.json @@ -94,7 +94,6 @@ "sync_all_busy": "Your playlists are already syncing with YouTube — try again when it finishes" }, "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" }, "railMenu": "Playlist actions", @@ -104,6 +103,24 @@ "dragHandleOf": "Reorder “{{title}}”", "selectRow": "Select “{{title}}”", "selectAllShown": "Select all the rows shown", + "selectedCount_one": "{{count}} selected", + "selectedCount_other": "{{count}} selected", + "selectedHidden_one": "{{count}} more hidden by the filter", + "selectedHidden_other": "{{count}} more hidden by the filter", + "clearSelection": "Clear selection", + "moveSelected": "Move to", + "removeSelected": "Remove", + "movedManyToast_one": "{{count}} video moved to “{{name}}”", + "movedManyToast_other": "{{count}} videos moved to “{{name}}”", + "removedManyToast_one": "{{count}} video removed from the playlist", + "removedManyToast_other": "{{count}} videos removed from the playlist", + "moveNoneLeft": "Nothing moved: they aren’t in this playlist any more", + "removeNoneLeft": "Nothing removed: they aren’t in this playlist any more", + "moveManyFailed": "Couldn't move the videos", + "removeManyFailed": "Couldn't remove the videos", + "undoMoveFailed": "Couldn't move the video back", + "undoMoveManyFailed": "Couldn't move the videos back", + "undoRemoveManyFailed": "Couldn't put the videos back", "dragBlockedSort": "Reordering by hand is off while a column sort or grouping is applied.", "dragBlockedFilter": "Reordering by hand is off while a filter is applied.", "clearView": "Reset view", diff --git a/frontend/src/i18n/locales/hu/keys.json b/frontend/src/i18n/locales/hu/keys.json index 3bb2f1df..417a8066 100644 --- a/frontend/src/i18n/locales/hu/keys.json +++ b/frontend/src/i18n/locales/hu/keys.json @@ -35,7 +35,7 @@ "listStep": "Következő / előző elem", "gridMove": "Mozgás a listában", "open": "Az aktuális elem megnyitása", - "listRemove": "Az aktuális elem eltávolítása a listából", + "listRemove": "A kijelölt sorok, vagy az aktuális eltávolítása", "listSelect": "Az aktuális elem kijelölése vagy levétele", "listClearSelection": "A kijelölés törlése", "cap": { diff --git a/frontend/src/i18n/locales/hu/playlists.json b/frontend/src/i18n/locales/hu/playlists.json index 74747a27..1e7f45ff 100644 --- a/frontend/src/i18n/locales/hu/playlists.json +++ b/frontend/src/i18n/locales/hu/playlists.json @@ -94,7 +94,6 @@ "sync_all_busy": "A listáid épp szinkronizálnak a YouTube-bal — próbáld újra, ha végzett" }, "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" }, "railMenu": "Lejátszási lista műveletei", @@ -104,6 +103,24 @@ "dragHandleOf": "„{{title}}” átrendezése", "selectRow": "„{{title}}” kijelölése", "selectAllShown": "Az összes látható sor kijelölése", + "selectedCount_one": "{{count}} kijelölve", + "selectedCount_other": "{{count}} kijelölve", + "selectedHidden_one": "további {{count}} a szűrő mögött", + "selectedHidden_other": "további {{count}} a szűrő mögött", + "clearSelection": "Kijelölés törlése", + "moveSelected": "Áthelyezés ide", + "removeSelected": "Eltávolítás", + "movedManyToast_one": "{{count}} videó áthelyezve ide: „{{name}}”", + "movedManyToast_other": "{{count}} videó áthelyezve ide: „{{name}}”", + "removedManyToast_one": "{{count}} videó eltávolítva a listáról", + "removedManyToast_other": "{{count}} videó eltávolítva a listáról", + "moveNoneLeft": "Nem történt áthelyezés: ezek már nincsenek ebben a listában", + "removeNoneLeft": "Nem történt eltávolítás: ezek már nincsenek ebben a listában", + "moveManyFailed": "Nem sikerült áthelyezni a videókat", + "removeManyFailed": "Nem sikerült eltávolítani a videókat", + "undoMoveFailed": "Nem sikerült visszahelyezni a videót", + "undoMoveManyFailed": "Nem sikerült visszahelyezni a videókat", + "undoRemoveManyFailed": "Nem sikerült visszatenni a videókat", "dragBlockedSort": "A kézi átrendezés ki van kapcsolva, amíg oszloprendezés vagy csoportosítás van érvényben.", "dragBlockedFilter": "A kézi átrendezés ki van kapcsolva, amíg szűrő van érvényben.", "clearView": "Nézet visszaállítása", diff --git a/frontend/src/lib/api/playlists.ts b/frontend/src/lib/api/playlists.ts index 5c156114..bf2ada0a 100644 --- a/frontend/src/lib/api/playlists.ts +++ b/frontend/src/lib/api/playlists.ts @@ -77,16 +77,58 @@ export const playlistsApi = { }), removeFromPlaylist: (id: number, videoId: string) => req(`/api/playlists/${id}/items/${videoId}`, { method: "DELETE" }), - // One request on purpose: add-then-remove from the client leaves the video in BOTH lists when - // the second call fails. The server does it in one transaction. - moveToPlaylist: ( + // One request on purpose: add-then-remove from the client leaves a video in BOTH lists when the + // second call fails. The server does it in one transaction, and says which landed in the target + // and which it already held: the Undo needs to tell them apart. + moveManyToPlaylist: ( id: number, - videoId: string, + videoIds: string[], targetPlaylistId: number, - ): Promise<{ ok: true; from_playlist_id: number; to_playlist_id: number; video_id: string }> => - req(`/api/playlists/${id}/items/${videoId}/move`, { + ): Promise<{ + ok: true; + from_playlist_id: number; + to_playlist_id: number; + moved: string[]; + already_there: string[]; + }> => + req(`/api/playlists/${id}/items/move-bulk`, { method: "POST", - body: JSON.stringify({ target_playlist_id: targetPlaylistId }), + body: JSON.stringify({ video_ids: videoIds, target_playlist_id: targetPlaylistId }), + }), + // The Undo of a move, back into `id`. `positions` pairs with `moved` + `alreadyThere`. + undoPlaylistMove: ( + id: number, + fromPlaylistId: number, + moved: string[], + alreadyThere: string[], + positions: number[], + ): Promise<{ ok: true; playlist_id: number; added: number }> => + req(`/api/playlists/${id}/items/undo-move`, { + method: "POST", + body: JSON.stringify({ + from_playlist_id: fromPlaylistId, + moved, + already_there: alreadyThere, + positions, + }), + }), + removeManyFromPlaylist: ( + id: number, + videoIds: string[], + ): Promise<{ ok: true; playlist_id: number; removed: number }> => + req(`/api/playlists/${id}/items/remove-bulk`, { + method: "POST", + body: JSON.stringify({ video_ids: videoIds }), + }), + // The Undo of a removal, in one request; an index in the stored order per video. + restoreToPlaylist: ( + id: number, + videoIds: string[], + positions: number[], + ): Promise<{ ok: true; playlist_id: number; added: number }> => + req(`/api/playlists/${id}/items/restore`, { + method: "POST", + body: JSON.stringify({ video_ids: videoIds, positions }), }), reorderPlaylist: (id: number, videoIds: string[]) => req(`/api/playlists/${id}/order`, { diff --git a/frontend/src/lib/listNav.select.dom.test.tsx b/frontend/src/lib/listNav.select.dom.test.tsx index 34a4fb9b..388ca702 100644 --- a/frontend/src/lib/listNav.select.dom.test.tsx +++ b/frontend/src/lib/listNav.select.dom.test.tsx @@ -10,9 +10,15 @@ import { useActiveScopes } from "./keymap"; afterEach(cleanup); -function Rows({ selection }: { selection?: NavSelection }) { +function Rows({ + selection, + onDelete, +}: { + selection?: NavSelection; + onDelete?: (index: number) => void; +}) { const ref = useRef(null); - useListNav(ref, { enabled: true, selection }); + useListNav(ref, { enabled: true, selection, onDelete }); return (
{[0, 1, 2].map((i) => ( @@ -39,6 +45,7 @@ function press(key: string, init: KeyboardEventInit = {}): KeyboardEvent { const selection = (cleared = true) => ({ toggle: vi.fn<(index: number) => void>(), clear: vi.fn(() => cleared), + removeSelected: vi.fn(() => false), }); describe("useListNav · selecting", () => { @@ -108,6 +115,25 @@ describe("useListNav · selecting", () => { expect(idle.clear).toHaveBeenCalledTimes(1); }); + it("has Delete go to the selection first, wherever the focus is", () => { + const s = selection(); + s.removeSelected.mockReturnValue(true); + const onDelete = vi.fn(); + render(); + expect(press("Delete").defaultPrevented).toBe(true); + expect(s.removeSelected).toHaveBeenCalledTimes(1); + expect(onDelete).not.toHaveBeenCalled(); + }); + + it("has Delete go on to the walked row when there is no selection", () => { + const s = selection(); + const onDelete = vi.fn(); + render(); + press("j"); + press("Delete"); + expect(onDelete).toHaveBeenCalledExactlyOnceWith(0); + }); + it("leaves both keys alone in a list that takes no selection", () => { render(); press("j"); diff --git a/frontend/src/lib/listNav.ts b/frontend/src/lib/listNav.ts index 89b452cc..289bfaf8 100644 --- a/frontend/src/lib/listNav.ts +++ b/frontend/src/lib/listNav.ts @@ -45,6 +45,9 @@ interface NavList { export interface NavSelection { readonly toggle: (index: number) => void; readonly clear: () => boolean; + /** Delete: takes the selection out, wherever the focus is. False when there is none, and Delete + * goes on to the item under the keyboard. */ + readonly removeSelected: () => boolean; } const lists = new Map(); @@ -432,6 +435,12 @@ function onKey(e: KeyboardEvent): void { // On the item root, like Enter — and only in a list that registered a remover. remove: (ev) => { const cur = current(); + for (const list of ordered()) { + if (!list.selection()?.removeSelected()) continue; + ev.preventDefault(); + if (cur) refocusSlot(cur.list, cur.index, cur.item); + return; + } const remove = cur?.list.remove(); if (!cur || !remove || document.activeElement !== cur.item) return; ev.preventDefault(); diff --git a/frontend/src/lib/tableColumns.ts b/frontend/src/lib/tableColumns.ts index f77d2a94..5aa04934 100644 --- a/frontend/src/lib/tableColumns.ts +++ b/frontend/src/lib/tableColumns.ts @@ -51,6 +51,9 @@ export interface Column { // one beside a panel: a secondary column that gives its room back to the name (the owner, // 2026-09-27: at 1024 the channel name had 60 px while About took as much). Same `@container` rule. tabletHidden?: boolean; + // …and one it leaves out while it is narrower than 24rem, a phone beside the nav rail: there the + // title kept 44 px. Only for a column whose job has another way (the grip: the row menu's moves). + phoneHidden?: boolean; } // The classes that hide a column below its width (`narrowHidden` / `tabletHidden`), for its cells and, @@ -71,14 +74,20 @@ const HIDE_BELOW = { col: " hidden @5xl:table-column", cue: " @5xl:hidden", }, + phone: { + cell: " hidden @sm:table-cell", + col: " hidden @sm:table-column", + cue: " @sm:hidden", + }, } as const; export function columnHideClass( - col: { narrowHidden?: boolean; tabletHidden?: boolean }, + col: { narrowHidden?: boolean; tabletHidden?: boolean; phoneHidden?: boolean }, part: "cell" | "col" | "cue" = "cell", ): string { if (col.narrowHidden) return HIDE_BELOW.narrow[part]; if (col.tabletHidden) return HIDE_BELOW.tablet[part]; + if (col.phoneHidden) return HIDE_BELOW.phone[part]; return ""; } diff --git a/frontend/src/lib/useRowSelection.dom.test.ts b/frontend/src/lib/useRowSelection.dom.test.ts index 17a318c9..836b9cc3 100644 --- a/frontend/src/lib/useRowSelection.dom.test.ts +++ b/frontend/src/lib/useRowSelection.dom.test.ts @@ -129,6 +129,22 @@ describe("useRowSelection", () => { expect(result.current.shownSelected).toEqual(["c"]); }); + it("counts the selected rows a filter hides", () => { + const { result, rerender } = setup(); + act(() => result.current.toggle("a")); + act(() => result.current.toggle("c")); + expect(result.current.hiddenCount).toBe(0); + rerender({ all: ABCDE, shown: ["c", "d"], scope: 1 }); + expect(result.current.hiddenCount).toBe(1); + }); + + it("ticks rows again on top of what is selected by then", () => { + const { result } = setup(); + act(() => result.current.toggle("e")); + act(() => result.current.select(["a", "b"])); + expect(result.current.shownSelected).toEqual(["a", "b", "e"]); + }); + it("says whether clearing did anything", () => { const { result } = setup(); let cleared = true; diff --git a/frontend/src/lib/useRowSelection.ts b/frontend/src/lib/useRowSelection.ts index 65f98c1f..cfa79c3e 100644 --- a/frontend/src/lib/useRowSelection.ts +++ b/frontend/src/lib/useRowSelection.ts @@ -59,8 +59,14 @@ export function useRowSelection(all: readonly string[], shown: readonly string[] return had; } + /** Tick these again: an action that took them off and failed. */ + function select(ids: readonly string[]) { + setSelected((now) => new Set([...now, ...ids])); + } + return { shownSelected, + hiddenCount: selected.size - shownSelected.length, isSelected: (id: string) => selected.has(id), headerState: shownSelected.length === 0 @@ -71,5 +77,6 @@ export function useRowSelection(all: readonly string[], shown: readonly string[] toggle, toggleAllShown, clear, + select, }; } diff --git a/frontend/src/modules/playlists/Playlists.dom.test.tsx b/frontend/src/modules/playlists/Playlists.dom.test.tsx index 19e4692d..92a27267 100644 --- a/frontend/src/modules/playlists/Playlists.dom.test.tsx +++ b/frontend/src/modules/playlists/Playlists.dom.test.tsx @@ -597,7 +597,7 @@ describe("Playlists — a change the server refused is taken back on screen", () // object for a deep-equal answer, and the page's re-seed effect is keyed on that object — so the // optimistic change used to stay on screen for good. it("puts back a row whose move failed", async () => { - vi.spyOn(api, "moveToPlaylist").mockRejectedValue( + vi.spyOn(api, "moveManyToPlaylist").mockRejectedValue( new HttpError(404, "gone", { code: "not_found", reason: "move_target" }), ); mount({ 1: [vid("V", 0), vid("W", 0)], 2: [] }, { lastMovePlaylistId: 2 }); @@ -611,7 +611,11 @@ describe("Playlists — a change the server refused is taken back on screen", () // Watch later's items ARE the bookmark: a video moved into it is saved wherever it is shown. it("marks a video moved into Watch later as saved", async () => { - vi.spyOn(api, "moveToPlaylist").mockResolvedValue(undefined as never); + vi.spyOn(api, "moveManyToPlaylist").mockResolvedValue({ + ok: true, + moved: ["V"], + already_there: [], + } as never); mount({ 1: [vid("V", 0)], [WATCH_LATER]: [] }, { lastMovePlaylistId: WATCH_LATER }); await rowMenu("Video V", "playlists.moveToRecent"); await waitFor(() => @@ -1127,3 +1131,362 @@ describe("Playlists — selecting rows", () => { expect(screen.queryAllByRole("checkbox", { name: /playlists\.select/ })).toEqual([]); }); }); + +describe("Playlists — acting on the selected rows", () => { + // A move remembers its target on the server, and an answer from nowhere would be the last toast. + beforeEach(() => void vi.spyOn(api, "savePrefs").mockResolvedValue({} as never)); + + const box = (title: string) => + screen.findByRole("checkbox", { name: `playlists.selectRow:${title}` }); + const tick = async (...titles: string[]) => { + for (const title of titles) fireEvent.click(await box(title)); + }; + const ticked = () => + screen + .getAllByRole("checkbox", { name: /playlists\.selectRow/ }) + .filter((b) => (b as HTMLInputElement).checked) + .map((b) => b.getAttribute("aria-label")); + const abc: Record = { 1: [vid("A", 0), vid("B", 0), vid("C", 0)], 2: [] }; + const toast = () => + h.notify.mock.calls[h.notify.mock.calls.length - 1]![0] as unknown as { + level: string; + message: unknown; + action?: { onClick: () => void }; + }; + const button = (name: string) => screen.findByRole("button", { name }); + const moved = (ids: string[], already_there: string[] = []) => ({ + ok: true, + from_playlist_id: 1, + to_playlist_id: 2, + moved: ids, + already_there, + }); + const press = (key: string) => + (document.activeElement ?? document.body).dispatchEvent( + new KeyboardEvent("keydown", { key, bubbles: true, cancelable: true }), + ); + + it("shows the band in place of the meta line only while a row is selected", async () => { + mount(abc); + await box("Video A"); + expect(screen.queryByText("playlists.selectedCount")).toBeNull(); + expect(screen.getByText("playlists.groupByChannel")).toBeTruthy(); + await tick("Video A"); + expect(await screen.findByText("playlists.selectedCount")).toBeTruthy(); + expect(screen.queryByText("playlists.groupByChannel")).toBeNull(); + fireEvent.click(await button("playlists.clearSelection")); + expect(screen.queryByText("playlists.selectedCount")).toBeNull(); + expect(await screen.findByText("playlists.groupByChannel")).toBeTruthy(); + }); + + it("offers the last target as one click, and every other list in the menu", async () => { + mount({ ...abc, 3: [] }, { lastMovePlaylistId: 2 }); + await tick("Video A"); + expect(await button("playlists.moveToRecent")).toBeTruthy(); + fireEvent.click(await button("playlists.moveSelected")); + await waitFor(() => + expect(screen.getAllByRole("menuitem", { hidden: true }).map((b) => b.textContent)).toEqual([ + "List 2", + "List 3", + ]), + ); + }); + + it("offers no one-click button before the first move", async () => { + mount(abc); + await tick("Video A"); + await button("playlists.moveSelected"); + expect(screen.queryByRole("button", { name: "playlists.moveToRecent" })).toBeNull(); + }); + + it("says why there is nowhere to move to", async () => { + mount({ 1: [vid("A", 0)] }); + await tick("Video A"); + fireEvent.click(await button("playlists.moveSelected")); + await waitFor(() => { + const row = screen.getByRole("menuitem", { hidden: true }); + expect(row.textContent).toContain("playlists.moveNoTargets"); + expect((row as HTMLButtonElement).disabled).toBe(true); + }); + }); + + it("moves the selected rows in one request, and clears the selection", async () => { + const lists = { ...abc }; + const move = vi.spyOn(api, "moveManyToPlaylist").mockImplementation((async () => { + lists[1] = lists[1]!.filter((v) => v.id === "B"); + return moved(["A", "C"]); + }) as never); + mount(lists, { lastMovePlaylistId: 2 }); + await tick("Video C", "Video A"); + fireEvent.click(await button("playlists.moveToRecent")); + await waitFor(() => expect(move).toHaveBeenCalledTimes(1)); + expect(move.mock.calls[0]![0]).toBe(1); + expect([...move.mock.calls[0]![1]].sort()).toEqual(["A", "C"]); + expect(move.mock.calls[0]![2]).toBe(2); + await waitFor(() => expect(rowsOnScreen()).toEqual(["Video B"])); + await waitFor(() => expect(toast().level).toBe("success")); + expect(screen.queryByText("playlists.selectedCount")).toBeNull(); + expect(toast().message).toEqual( + msg("playlists.movedManyToast", { count: 2, name: raw("List 2") }), + ); + }); + + it("undoes a move back to the places, leaving alone what the target held before", async () => { + vi.spyOn(api, "moveManyToPlaylist").mockResolvedValue(moved(["A"], ["C"]) as never); + const undo = vi.spyOn(api, "undoPlaylistMove").mockResolvedValue({ ok: true } as never); + mount(abc, { lastMovePlaylistId: 2 }); + await tick("Video A", "Video C"); + fireEvent.click(await button("playlists.moveToRecent")); + await waitFor(() => expect(toast().action).toBeTruthy()); + vi.spyOn(api, "playlist").mockResolvedValue(list(1, [vid("B", 0)]) as never); + toast().action!.onClick(); + await waitFor(() => expect(undo).toHaveBeenCalledWith(1, 2, ["A"], ["C"], [0, 2])); + }); + + it("leaves out of the undo a video that is back in the source by then", async () => { + vi.spyOn(api, "moveManyToPlaylist").mockResolvedValue(moved(["A", "B"], ["C"]) as never); + const undo = vi.spyOn(api, "undoPlaylistMove").mockResolvedValue({ ok: true } as never); + mount(abc, { lastMovePlaylistId: 2 }); + await tick("Video A", "Video B", "Video C"); + fireEvent.click(await button("playlists.moveToRecent")); + await waitFor(() => expect(toast().action).toBeTruthy()); + vi.spyOn(api, "playlist").mockResolvedValue(list(1, [vid("B", 0)]) as never); + toast().action!.onClick(); + await waitFor(() => expect(undo).toHaveBeenCalledWith(1, 2, ["A"], ["C"], [0, 2])); + }); + + it("says when the undo of a move failed", async () => { + vi.spyOn(api, "moveManyToPlaylist").mockResolvedValue(moved(["A", "B"]) as never); + vi.spyOn(api, "undoPlaylistMove").mockRejectedValue(new HttpError(500, "boom")); + mount(abc, { lastMovePlaylistId: 2 }); + await tick("Video A", "Video B"); + fireEvent.click(await button("playlists.moveToRecent")); + await waitFor(() => expect(toast().action).toBeTruthy()); + vi.spyOn(api, "playlist").mockResolvedValue(list(1, [vid("C", 0)]) as never); + toast().action!.onClick(); + await waitFor(() => expect(toast().level).toBe("error")); + }); + + it("puts the rows back, still selected, when the move failed", async () => { + vi.spyOn(api, "moveManyToPlaylist").mockRejectedValue(new HttpError(500, "boom")); + mount(abc, { lastMovePlaylistId: 2 }); + await tick("Video A", "Video C"); + fireEvent.click(await button("playlists.moveToRecent")); + await waitFor(() => expect(toast().level).toBe("error")); + await waitFor(() => expect(rowsOnScreen()).toEqual(["Video A", "Video B", "Video C"])); + await waitFor(() => + expect(ticked()).toEqual(["playlists.selectRow:Video A", "playlists.selectRow:Video C"]), + ); + }); + + it("says so when none of them was in the list any more, with no Undo to offer", async () => { + vi.spyOn(api, "moveManyToPlaylist").mockResolvedValue(moved([]) as never); + mount(abc, { lastMovePlaylistId: 2 }); + await tick("Video A", "Video B"); + fireEvent.click(await button("playlists.moveToRecent")); + await waitFor(() => expect(h.notify).toHaveBeenCalled()); + expect(toast().message).toEqual(msg("playlists.moveNoneLeft")); + expect(toast().action).toBeUndefined(); + }); + + it("counts what really moved, not what was asked for", async () => { + vi.spyOn(api, "moveManyToPlaylist").mockResolvedValue(moved(["A", "B"]) as never); + mount(abc, { lastMovePlaylistId: 2 }); + await tick("Video A", "Video B", "Video C"); + fireEvent.click(await button("playlists.moveToRecent")); + await waitFor(() => expect(h.notify).toHaveBeenCalled()); + expect(toast().message).toEqual( + msg("playlists.movedManyToast", { count: 2, name: raw("List 2") }), + ); + }); + + it("removes the selected rows in one request, with an Undo back to the places", async () => { + const lists = { ...abc }; + const remove = vi.spyOn(api, "removeManyFromPlaylist").mockImplementation((async () => { + lists[1] = lists[1]!.filter((v) => v.id === "B"); + return { ok: true, playlist_id: 1, removed: 2 }; + }) as never); + const restore = vi.spyOn(api, "restoreToPlaylist").mockResolvedValue({ ok: true } as never); + mount(lists); + await tick("Video A", "Video C"); + fireEvent.click(await button("playlists.removeSelected")); + await waitFor(() => expect(remove).toHaveBeenCalledTimes(1)); + expect([...remove.mock.calls[0]![1]].sort()).toEqual(["A", "C"]); + await waitFor(() => expect(rowsOnScreen()).toEqual(["Video B"])); + await waitFor(() => expect(toast().action).toBeTruthy()); + expect(toast().message).toEqual(msg("playlists.removedManyToast", { count: 2 })); + vi.spyOn(api, "playlist").mockResolvedValue(list(1, [vid("B", 0)]) as never); + toast().action!.onClick(); + await waitFor(() => expect(restore).toHaveBeenCalledWith(1, ["A", "C"], [0, 2])); + }); + + it("puts the rows back, still selected, when the removal failed", async () => { + vi.spyOn(api, "removeManyFromPlaylist").mockRejectedValue(new HttpError(500, "boom")); + mount(abc); + await tick("Video B"); + fireEvent.click(await button("playlists.removeSelected")); + await waitFor(() => expect(toast().level).toBe("error")); + await waitFor(() => expect(rowsOnScreen()).toEqual(["Video A", "Video B", "Video C"])); + await waitFor(() => expect(ticked()).toEqual(["playlists.selectRow:Video B"])); + }); + + it("names the one video when one was removed", async () => { + vi.spyOn(api, "removeManyFromPlaylist").mockResolvedValue({ + ok: true, + playlist_id: 1, + removed: 1, + } as never); + mount(abc); + await tick("Video B"); + fireEvent.click(await button("playlists.removeSelected")); + await waitFor(() => expect(toast().action).toBeTruthy()); + expect(toast().message).toEqual(msg("playlists.removedToast", { title: raw("Video B") })); + }); + + it("has Delete take the selection when there is one, with the focus on a box", async () => { + const remove = vi + .spyOn(api, "removeManyFromPlaylist") + .mockResolvedValue({ ok: true, playlist_id: 1, removed: 2 } as never); + const single = vi.spyOn(api, "removeFromPlaylist").mockResolvedValue({ ok: true } as never); + mount(abc); + await tick("Video A", "Video B"); + (await box("Video B")).focus(); + await act(async () => press("Delete")); + await waitFor(() => expect(remove).toHaveBeenCalledTimes(1)); + expect(single).not.toHaveBeenCalled(); + }); + + it("has Delete take the walked row when nothing is selected", async () => { + const remove = vi.spyOn(api, "removeManyFromPlaylist").mockResolvedValue({} as never); + const single = vi.spyOn(api, "removeFromPlaylist").mockResolvedValue({ ok: true } as never); + mount(abc); + await box("Video A"); + press("j"); + await act(async () => press("Delete")); + await waitFor(() => expect(single).toHaveBeenCalledWith(1, "A")); + expect(remove).not.toHaveBeenCalled(); + }); + + it("keeps a row's own menu on that row, and leaves the selection alone", async () => { + const lists = { ...abc }; + const move = vi.spyOn(api, "moveManyToPlaylist").mockImplementation((async () => { + lists[1] = lists[1]!.filter((v) => v.id !== "B"); + return moved(["B"]); + }) as never); + mount(lists, { lastMovePlaylistId: 2 }); + await tick("Video A"); + await rowMenu("Video B", "playlists.moveToRecent"); + await waitFor(() => expect(move).toHaveBeenCalledWith(1, ["B"], 2)); + await waitFor(() => expect(rowsOnScreen()).toEqual(["Video A", "Video C"])); + expect(ticked()).toEqual(["playlists.selectRow:Video A"]); + }); + + it("gives a single row's move the same Undo", async () => { + vi.spyOn(api, "moveManyToPlaylist").mockResolvedValue(moved(["B"]) as never); + const undo = vi.spyOn(api, "undoPlaylistMove").mockResolvedValue({ ok: true } as never); + mount(abc, { lastMovePlaylistId: 2 }); + await rowMenu("Video B", "playlists.moveToRecent"); + await waitFor(() => expect(toast().action).toBeTruthy()); + expect(toast().message).toEqual( + msg("playlists.movedToast", { title: raw("Video B"), name: raw("List 2") }), + ); + vi.spyOn(api, "playlist").mockResolvedValue(list(1, [vid("A", 0), vid("C", 0)]) as never); + toast().action!.onClick(); + await waitFor(() => expect(undo).toHaveBeenCalledWith(1, 2, ["B"], [], [1])); + }); + + it("acts on the rows shown, says how many a filter hides, and keeps the view's reset", async () => { + const remove = vi + .spyOn(api, "removeManyFromPlaylist") + .mockResolvedValue({ ok: true, playlist_id: 1, removed: 1 } as never); + mount(abc); + await tick("Video A", "Video B"); + fireEvent.click(screen.getAllByRole("button", { name: "datatable.filter" })[0]!); + fireEvent.change(await screen.findByPlaceholderText("playlists.colTitle"), { + target: { value: "Video B" }, + }); + expect(await screen.findByText("playlists.selectedHidden")).toBeTruthy(); + expect(await button("playlists.clearView")).toBeTruthy(); + fireEvent.click(await button("playlists.removeSelected")); + await waitFor(() => expect(remove).toHaveBeenCalledWith(1, ["B"])); + }); + + it("forgets the ticks a filter hid once it acted, too", async () => { + vi.spyOn(api, "removeManyFromPlaylist").mockResolvedValue({ + ok: true, + playlist_id: 1, + removed: 1, + } as never); + mount(abc); + await tick("Video A", "Video B"); + fireEvent.click(screen.getAllByRole("button", { name: "datatable.filter" })[0]!); + fireEvent.change(await screen.findByPlaceholderText("playlists.colTitle"), { + target: { value: "Video B" }, + }); + await screen.findByText("playlists.selectedHidden"); + fireEvent.click(await button("playlists.removeSelected")); + await waitFor(() => expect(toast().action).toBeTruthy()); + expect(screen.queryByText("playlists.selectedHidden")).toBeNull(); + expect(screen.queryByText("playlists.selectedCount")).toBeNull(); + }); + + it("offers nothing to act with while every selected row is hidden, only the way out", async () => { + mount(abc); + await tick("Video A"); + fireEvent.click(screen.getAllByRole("button", { name: "datatable.filter" })[0]!); + fireEvent.change(await screen.findByPlaceholderText("playlists.colTitle"), { + target: { value: "Video B" }, + }); + expect(await screen.findByText("playlists.selectedHidden")).toBeTruthy(); + expect(((await button("playlists.removeSelected")) as HTMLButtonElement).disabled).toBe(true); + expect(((await button("playlists.moveSelected")) as HTMLButtonElement).disabled).toBe(true); + expect(((await button("playlists.clearSelection")) as HTMLButtonElement).disabled).toBe(false); + }); + + it("hands the focus to the header's box when the band goes from under it", async () => { + mount(abc); + await tick("Video A"); + const clear = await button("playlists.clearSelection"); + clear.focus(); + fireEvent.click(clear); + await waitFor(() => + expect(document.activeElement).toBe( + screen.getByRole("checkbox", { name: "playlists.selectAllShown" }), + ), + ); + }); + + it("hands the focus to the list's menu when the band's action emptied the list", async () => { + vi.spyOn(api, "removeManyFromPlaylist").mockResolvedValue({ + ok: true, + playlist_id: 1, + removed: 1, + } as never); + mount({ 1: [vid("A", 0)] }); + await tick("Video A"); + const remove = await button("playlists.removeSelected"); + remove.focus(); + fireEvent.click(remove); + await waitFor(() => expect(toast().action).toBeTruthy()); + expect(document.activeElement).toBe(screen.getByRole("button", { name: "playlists.railMenu" })); + }); + + it("leaves the focus alone when the band goes and it was elsewhere", async () => { + mount(abc); + await tick("Video A"); + const row = await box("Video B"); + row.focus(); + await act(async () => press("Escape")); + expect(screen.queryByText("playlists.selectedCount")).toBeNull(); + expect(document.activeElement).toBe(row); + }); + + it("drops the grip below a phone-wide table: the row menu keeps the moves", async () => { + mount(abc); + await box("Video A"); + const grip = screen.getByRole("button", { name: "playlists.dragHandleOf:Video A" }); + const cell = grip.closest("td")!; + expect(cell.classList.contains("hidden")).toBe(true); + expect(cell.classList.contains("@sm:table-cell")).toBe(true); + }); +}); diff --git a/frontend/src/modules/playlists/Playlists.tsx b/frontend/src/modules/playlists/Playlists.tsx index 5120f698..54bb6387 100644 --- a/frontend/src/modules/playlists/Playlists.tsx +++ b/frontend/src/modules/playlists/Playlists.tsx @@ -6,8 +6,11 @@ import { qk } from "../../lib/queryKeys"; import { hasResumePoint } from "../../lib/api/feed"; import { addPlaylistItem, - movePlaylistItem, + movePlaylistItems, removePlaylistItem, + removePlaylistItems, + restorePlaylistItems, + undoPlaylistMove, clearVideoProgress, writeVideoState, } from "../../features/video/videoWrites"; @@ -83,6 +86,7 @@ import { import { GRIP_KEY, playlistColumns, selectColumn } from "./playlistColumns"; import { useSortableA11y, useSortableItem } from "../../components/ui/sortableA11y"; import { DragHandle } from "../../components/ui/DragHandle"; +import { RowSelectBox } from "../../components/ui/RowSelectBox"; import { useOrderItems } from "../../components/ui/useOrderItems"; import { Input } from "../../components/ui/Input"; import { CheckRow } from "../../components/ui/NativeControls"; @@ -93,6 +97,7 @@ import { PageHeading } from "../../components/ui/Headings"; import { useCopyItems } from "../../components/ui/copyItems"; import RemoteImage from "../../components/ui/RemoteImage"; import PlaylistsHeader from "./PlaylistsHeader"; +import { SelectionBand } from "./SelectionBand"; // The playlist detail is a HYBRID table: real sortable/filterable column headers over a // body that is still a drag-reorderable @dnd-kit list. @@ -116,6 +121,13 @@ const idsKey = (items: Video[]) => .sort() .join(","); +const moveTargetIcon = (pl: Playlist) => + pl.source === "youtube" || pl.yt_playlist_id ? ( + + ) : ( + + ); + function Row({ video, index, @@ -455,12 +467,24 @@ function PlaylistDetail() { shown.map((v) => v.id), selectedId, ); + const selectAllRef = useRef(null); + const bandRef = useRef(null); + // The band's own "Move to" list, the row menu's with no row behind it. + const moveMenu = useContextMenu<"selection">(); const tableColumns = canEdit ? [ selectColumn({ t, - state: selection.headerState, - onToggleAll: selection.toggleAllShown, + allBox: ( + + ), isSelected: selection.isSelected, onToggle: selection.toggle, }), @@ -469,6 +493,8 @@ function PlaylistDetail() { : columns; const viewActive = !isStoredSort(viewSort) || groupBy || filterActive; + // While anything is selected, shown or not: a tick only a filter hides still needs its way out. + const bandUp = canEdit && selection.shownSelected.length + selection.hiddenCount > 0; // Saving a FILTERED view would drop every hidden item from the playlist, so the button only // offers itself when the view is a pure reordering of the whole list. const canSaveOrder = canEdit && !filterActive && !sameOrder(shown, items); @@ -637,42 +663,207 @@ function PlaylistDetail() { linkedOutOfReach && selectedId === railMovedTo ? linkedOver : selectedId, ); - async function moveItemTo(video: Video, target: Playlist) { - if (selectedId == null) return; - // Optimistic, exactly like removeItem: from THIS list's point of view a move is a removal, and - // rebasing keeps the undo stack valid for what's left instead of throwing it away. - const next = items.filter((v) => v.id !== video.id); - order.rebase((list) => list.filter((v) => v.id !== video.id)); - lastSetRef.current = `${selectedId}:${idsKey(next)}`; - // Outside the `try`: a `??` inside it makes the React Compiler skip the component. - const title = video.title ?? ""; - try { - // ONE request. The two-call version (add there, remove here) leaves the video in BOTH lists - // whenever the second call fails — see the endpoint's docstring. - await movePlaylistItem(qc, listRef(selectedId), video.id, target); - // Remembered only on SUCCESS: a shortcut that points at a move which failed would be a - // shortcut to failing again. - setLastMoveTargetId(target.id); - notify({ - level: "success", - message: msg("playlists.movedToast", { title: raw(title), name: playlistNameText(target) }), - }); - } catch (e) { - // NOT a silent revert. `req` deliberately leaves 401/403/404 to the caller as control flow, and - // every move failure a user can actually reach is a 404 (the 400 `same_playlist` case is already - // impossible — the submenu never offers the current list). So the row used to snap back with no - // explanation and the shipped moveErrors strings were unreachable. - // A fault `req` already put in its error dialog is not toasted again — `notifyActionError` - // checks (a review found it reported twice). - notifyActionError(e, msg("playlists.moveFailed")); - restoreFromCache(selectedId); // the row is still in this list - } - // BOTH sides changed, and so did this video's membership. `qk.playlist(selectedId)` is an exact - // key, so it cannot cover the target — invalidate the whole `["playlist"]` prefix, exactly as - // AddToPlaylist does for the same reason. + // Off the screen at once, and out of the cached list too, for the reason `removeItem` gives. Returns + // the cached list as it stood: the place an Undo goes back to, and what a failure puts back. + function takeOut(listId: number, ids: readonly string[]) { + const gone = new Set(ids); + const next = order.value.filter((v) => !gone.has(v.id)); + order.rebase((list) => list.filter((v) => !gone.has(v.id))); + lastSetRef.current = `${listId}:${idsKey(next)}`; + const before = qc.getQueryData(qk.playlist(listId)); + qc.setQueryData(qk.playlist(listId), (old) => + old ? { ...old, items: old.items.filter((v) => !gone.has(v.id)) } : old, + ); + return before; + } + // The write failed: the rows come back now (see `restoreFromCache`), ticked again if they were, so + // the reader can try once more without picking them all over. + function putBack(listId: number, before: PlaylistDetail | undefined, ticked: readonly string[]) { + if (before) qc.setQueryData(qk.playlist(listId), before); + restoreFromCache(listId); + if (listId === selectedRef.current) selection.select(ticked); + } + // BOTH lists of a move changed, and so did these videos' membership. `qk.playlist(id)` is an exact + // key, so it cannot cover the target: the whole `["playlist"]` prefix, as AddToPlaylist does. + function refreshAfterEdit(ids: readonly string[]) { qc.invalidateQueries({ queryKey: qk.playlists() }); qc.invalidateQueries({ queryKey: qk.playlist() }); - qc.invalidateQueries({ queryKey: qk.playlistMembership(video.id) }); + for (const id of ids) qc.invalidateQueries({ queryKey: qk.playlistMembership(id) }); + } + const titleOf = (rows: Video[], id: string | undefined) => + raw(rows.find((v) => v.id === id)?.title ?? ""); + + /** Move rows into another list, in ONE request (see the endpoint). `ticked`: the ones the selection + * held, which a failure ticks again. */ + async function moveRows(rows: Video[], target: Playlist, ticked: readonly string[] = []) { + if (selectedId == null || !rows.length) return; + const listId = selectedId; + const ids = rows.map((v) => v.id); + const from = listRef(listId); + const before = takeOut(listId, ids); + const failTitle = msg(rows.length > 1 ? "playlists.moveManyFailed" : "playlists.moveFailed"); + let done: Awaited> | null = null; + try { + done = await movePlaylistItems(qc, from, ids, target); + } catch (e) { + // NOT a silent revert: a target that is gone answers 404, which `req` leaves to the caller. A + // fault `req` already put in its error dialog is not toasted again (`notifyActionError` checks). + notifyActionError(e, failTitle); + } + if (!done) { + putBack(listId, before, ticked); + refreshAfterEdit(ids); + return; + } + const { moved, already_there: alreadyThere } = done; + const left = [...moved, ...alreadyThere]; + refreshAfterEdit(ids); + // Remembered only on SUCCESS: a shortcut that points at a move which failed would be a shortcut + // to failing again. + setLastMoveTargetId(target.id); + if (!left.length) { + notify({ level: "warning", message: msg("playlists.moveNoneLeft") }); + return; + } + const name = playlistNameText(target); + const storedBefore = before?.items.map((v) => v.id) ?? []; + notify({ + level: "success", + message: + left.length === 1 + ? msg("playlists.movedToast", { title: titleOf(rows, left[0]), name }) + : msg("playlists.movedManyToast", { count: left.length, name }), + action: { + label: msg("common.undo"), + // The places worked out against the list as it is NOW, as in `removeItem`. A video back in + // the source by then stays where it is, in both lists. + onClick: () => + void qc + .fetchQuery({ + queryKey: qk.playlist(listId), + queryFn: () => api.playlist(listId), + staleTime: 0, + }) + .then((now) => { + const back = reinsertAt( + storedBefore, + left, + now.items.map((v) => v.id), + ); + const place = new Map(back.keys.map((id, i) => [id, back.positions[i]!])); + const keep = (list: string[]) => list.filter((id) => place.has(id)); + const undoMoved = keep(moved); + const undoHeld = keep(alreadyThere); + if (back.keys.length) + return undoPlaylistMove(qc, from, target, { + moved: undoMoved, + already_there: undoHeld, + positions: [...undoMoved, ...undoHeld].map((id) => place.get(id)!), + }); + }) + .catch((e: unknown) => + notifyActionError( + e, + msg(left.length > 1 ? "playlists.undoMoveManyFailed" : "playlists.undoMoveFailed"), + ), + ) + .finally(() => refreshAfterEdit(left)), + }, + }); + } + + /** Remove rows in ONE request, with an Undo that puts them back at their places in one too. */ + async function removeRows(rows: Video[], ticked: readonly string[]) { + if (selectedId == null || !rows.length) return; + const listId = selectedId; + const ids = rows.map((v) => v.id); + const list = listRef(listId); + const before = takeOut(listId, ids); + const failTitle = msg( + rows.length > 1 ? "playlists.removeManyFailed" : "playlists.removeFailed", + ); + let removed: number | null = null; + try { + removed = (await removePlaylistItems(qc, list, ids)).removed; + } catch (e) { + notifyActionError(e, failTitle); + } + refreshAfterEdit(ids); + if (removed == null) { + putBack(listId, before, ticked); + return; + } + if (removed === 0) { + notify({ level: "warning", message: msg("playlists.removeNoneLeft") }); + return; + } + const storedBefore = before?.items.map((v) => v.id) ?? []; + notify({ + level: "success", + message: + removed === 1 && rows.length === 1 + ? msg("playlists.removedToast", { title: titleOf(rows, ids[0]) }) + : msg("playlists.removedManyToast", { count: removed }), + action: { + label: msg("common.undo"), + onClick: () => + void qc + .fetchQuery({ + queryKey: qk.playlist(listId), + queryFn: () => api.playlist(listId), + staleTime: 0, + }) + .then((now) => { + const back = reinsertAt( + storedBefore, + ids, + now.items.map((v) => v.id), + ); + if (back.keys.length) + return restorePlaylistItems(qc, list, back.keys, back.positions); + }) + .catch((e: unknown) => + notifyActionError( + e, + msg( + ids.length > 1 ? "playlists.undoRemoveManyFailed" : "playlists.undoRemoveFailed", + ), + ), + ) + .finally(() => refreshAfterEdit(ids)), + }, + }); + } + + // The band goes when the selection does. A focus inside it would fall to , and the next Tab + // start over at the top of the page: the header's box takes it, or, when the action leaves no row + // and so no box, the list's "…". + function leaveBand(emptied = false) { + if (!bandRef.current?.contains(document.activeElement)) return; + (emptied ? renameTarget.current : selectAllRef.current)?.focus(); + } + // The selection goes as the action starts: its rows are leaving, and a tick a filter hides would + // come back later on a row the reader has forgotten about. A failure ticks the rows acted on again. + function selectionRows() { + const rows = shown.filter((v) => selection.isSelected(v.id)); + leaveBand(rows.length === items.length); + selection.clear(); + return rows; + } + function moveSelected(target: Playlist) { + const rows = selectionRows(); + void moveRows( + rows, + target, + rows.map((v) => v.id), + ); + } + function removeSelected() { + const rows = selectionRows(); + void removeRows( + rows, + rows.map((v) => v.id), + ); } // Keyboard/menu reordering, for the same stored order the drag handle writes — so it goes through @@ -790,7 +981,7 @@ function PlaylistDetail() { label: t("playlists.moveToRecent", { name: plName(recentMoveTarget) }), icon: , separatorBefore: true, - onSelect: () => void moveItemTo(ctxRow, recentMoveTarget), + onSelect: () => void moveRows([ctxRow], recentMoveTarget), }, ] : []), @@ -804,13 +995,8 @@ function PlaylistDetail() { items: moveTargets.map((pl) => ({ id: `move-${pl.id}`, label: plName(pl), - icon: - pl.source === "youtube" || pl.yt_playlist_id ? ( - - ) : ( - - ), - onSelect: () => void moveItemTo(ctxRow, pl), + icon: moveTargetIcon(pl), + onSelect: () => void moveRows([ctxRow], pl), })), } : // Kept visible and disabled rather than dropped: an action that vanishes teaches @@ -843,6 +1029,24 @@ function PlaylistDetail() { : []), ]; + const moveMenuItems: ActionMenuItem[] = moveTargets.length + ? moveTargets.map((pl) => ({ + id: `move-${pl.id}`, + label: plName(pl), + icon: moveTargetIcon(pl), + onSelect: () => moveSelected(pl), + })) + : [ + { + id: "move", + label: t("playlists.moveTo"), + icon: , + disabled: true, + note: t("playlists.moveNoTargets"), + onSelect: () => {}, + }, + ]; + const headerPl = headerMenu.target; const headerItems: ActionMenuItem[] = !headerPl ? [] @@ -1035,64 +1239,91 @@ function PlaylistDetail() {
-
- - {t("playlists.itemCount", { count: items.length })} - {totalSeconds > 0 && ` · ${formatRuntime(totalSeconds, lang)}`} - {filterActive && ` · ${t("playlists.filteredCount", { count: shown.length })}`} - - {(items.length > 1 || viewActive) && ( - <> - - - {t("playlists.groupByChannel")} - - {viewActive && ( - - )} - {canSaveOrder && ( - - )} - {/* ONE sentence that names the state. Three loose texts used to pile up here + {t("playlists.groupByChannel")} + + {viewActive && ( + + )} + {canSaveOrder && ( + + )} + {/* ONE sentence that names the state. Three loose texts used to pile up here (reset link + save hint + "reordering is off"), and none of them answered the question a user actually has — "did I just change my playlist?". Worse, sorting a list whose stored order ALREADY matches the sort produced a warning, no Save button and no visible change, which reads as a malfunction. Now the line says which of the three situations you are in; the disabled grip still explains itself on hover/focus. */} - {viewActive && {viewStatus}} - {canEdit && ( -
- -
- )} - - )} -
+ {viewActive && {viewStatus}} + {canEdit && ( +
+ +
+ )} + + )} + + )} @@ -1160,7 +1391,15 @@ function PlaylistDetail() { const v = shownRef.current[i]; if (v) selection.toggle(v.id); }, - clear: selection.clear, + clear: () => { + leaveBand(); + return selection.clear(); + }, + removeSelected: () => { + if (!selection.shownSelected.length) return false; + removeSelected(); + return true; + }, } : undefined } @@ -1192,6 +1431,11 @@ function PlaylistDetail() { {/* One surface for the whole table, outside the rows — it anchors to the pointer, not the row. */} +