From b636ef38bcaceeff7d51f5b62ca5a920b645d190 Mon Sep 17 00:00:00 2001 From: npeter83 Date: Mon, 2 Nov 2026 22:15:13 +0100 Subject: [PATCH] fix(playlists): overlapping writes, and what the second review found A read from before a write no longer brings a row back: a write calls off the read on its way, what lands while one is out stays off the screen, and the last write out does the refresh. A failure puts back its own rows, an accepted reorder brings back none, and an Undo reads past the cache. A list the cache has dropped is read before a removal, so its Undo has places. undo-move refuses the same list twice, positions cannot repeat, and undo-move and the Plex key bodies refuse a body without their videos. Focus goes to the header's box when the last shown row or Save this order goes. --- backend/app/reader/service.py | 4 +- backend/app/routes/playlists.py | 20 +- backend/app/routes/plex/playlists.py | 4 +- backend/tests/test_playlist_bulk.py | 55 +++ .../tests/test_plex_playlist_add_positions.py | 18 + docs/DESIGN.md | 7 +- frontend/src/i18n/locales/en/playlists.json | 9 +- frontend/src/i18n/locales/hu/keys.json | 2 +- frontend/src/i18n/locales/hu/playlists.json | 9 +- frontend/src/lib/api/playlists.ts | 2 - .../modules/playlists/Playlists.dom.test.tsx | 340 +++++++++++++++++- frontend/src/modules/playlists/Playlists.tsx | 179 ++++++--- 12 files changed, 578 insertions(+), 71 deletions(-) diff --git a/backend/app/reader/service.py b/backend/app/reader/service.py index 006fec5a..930f2321 100644 --- a/backend/app/reader/service.py +++ b/backend/app/reader/service.py @@ -559,7 +559,9 @@ def undo_move( `dst`, which held them before. A moved video `dst` no longer holds has gone on from there, and bringing it back would leave it in lists the reader never put it in, so it stays out, as a move skips a video a concurrent edit took out. `positions` pairs with `moved` + `already_there`. - Says how many came back.""" + Says how many came back. Raises `MoveRefused` for the same list twice, as a move does.""" + if src.id == dst.id: + raise MoveRefused("same_playlist") _lock_both(db, src, dst) wanted = set(moved) rows = {it.video_id: it for it in queries.items(db, dst.id) if it.video_id in wanted} diff --git a/backend/app/routes/playlists.py b/backend/app/routes/playlists.py index d5c7e82b..5622dd2f 100644 --- a/backend/app/routes/playlists.py +++ b/backend/app/routes/playlists.py @@ -420,11 +420,14 @@ class VideoIdsIn(BaseModel): def _paired(video_count: int, positions: list[int]) -> None: - """One place per video, none before the start: both undo bodies come back from the client.""" + """One place per video, none before the start and none twice: both undo bodies come back from + the client, and the insert's arithmetic counts on places that only rise.""" if len(positions) != video_count: raise ValueError("positions must pair one-to-one with the videos") if any(at < 0 for at in positions): raise ValueError("positions cannot be negative") + if len(set(positions)) != len(positions): + raise ValueError("positions cannot repeat") class RestoreItemsIn(VideoIdsIn): @@ -443,10 +446,11 @@ class MoveItemsIn(VideoIdsIn): class UndoMoveIn(BaseModel): from_playlist_id: int - moved: list[str] = [] + # Required, as `video_ids` is. An empty one is still a real body: the target held them all. + moved: list[str] already_there: list[str] = [] # One per video of `moved` + `already_there`, in that order. - positions: list[int] = [] + positions: list[int] @model_validator(mode="after") def _one_position_per_video(self): @@ -459,6 +463,10 @@ def _list_gone(e: service.ListGone) -> HTTPException: return not_found("playlist") if e.source else not_found("move_target") +def _move_refused(e: service.MoveRefused) -> HTTPException: + return HTTPException(status_code=400, detail={"code": "playlist_move", "reason": e.reason}) + + def _other_list(db: Session, user: User, playlist_id: int) -> "Playlist": """The other list of a move. Its own reason: "this playlist doesn't exist any more" about a list that is gone was read as the one on screen, which is still there.""" @@ -515,9 +523,7 @@ def move_items( try: moved, already_there = service.move_items(db, src, dst, body.video_ids) except service.MoveRefused as e: - raise HTTPException( - status_code=400, detail={"code": "playlist_move", "reason": e.reason} - ) from None + raise _move_refused(e) from None except service.ListGone as e: raise _list_gone(e) from None db.commit() @@ -542,6 +548,8 @@ def undo_move( dst = _other_list(db, user, body.from_playlist_id) try: added = service.undo_move(db, src, dst, body.moved, body.already_there, body.positions) + except service.MoveRefused as e: + raise _move_refused(e) from None except service.ListGone as e: raise _list_gone(e) from None db.commit() diff --git a/backend/app/routes/plex/playlists.py b/backend/app/routes/plex/playlists.py index 71b5d453..3c782494 100644 --- a/backend/app/routes/plex/playlists.py +++ b/backend/app/routes/plex/playlists.py @@ -183,7 +183,9 @@ def playlist_remove_item( class PlexItemKeysIn(BaseModel): - item_rating_keys: list[str] = [] + # Required: with a default, a misnamed key answered 200 having acted on no item, and a reorder + # took it for an empty order. + item_rating_keys: list[str] class PlexItemAddBulkIn(PlexItemKeysIn): diff --git a/backend/tests/test_playlist_bulk.py b/backend/tests/test_playlist_bulk.py index f11e9fc9..53ddcb90 100644 --- a/backend/tests/test_playlist_bulk.py +++ b/backend/tests/test_playlist_bulk.py @@ -437,6 +437,7 @@ def test_a_one_list_write_finds_its_list_under_the_lock( [ ("items/remove-bulk", {"ids": ["a"]}), ("items/move-bulk", {"ids": ["a"], "target_playlist_id": 1}), + ("items/undo-move", {"ids": ["a"], "from_playlist_id": 1}), ], ) def test_a_body_without_its_videos_is_refused_rather_than_acting_on_none( @@ -448,6 +449,60 @@ def test_a_body_without_its_videos_is_refused_rather_than_acting_on_none( assert as_user(user).post(f"/api/playlists/{pl.id}/{path}", json=body).status_code == 422 +def test_an_undo_of_only_videos_the_target_held_names_no_moved_ones(db, make_user, as_user): + """An empty `moved` is a real body: every video of the move was already in the target.""" + user = make_user() + _seed_videos(db, ["a"]) + src = _playlist(db, user, "Source", []) + dst = _playlist(db, user, "Target", ["a"]) + + r = as_user(user).post( + f"/api/playlists/{src.id}/items/undo-move", + json={"from_playlist_id": dst.id, "moved": [], "already_there": ["a"], "positions": [0]}, + ) + + assert r.json()["added"] == 1 + assert _ids(db, src.id) == ["a"] + assert _ids(db, dst.id) == ["a"] + + +def test_an_undo_within_one_list_is_refused_and_keeps_its_videos(db, make_user, as_user): + """A move refuses the same list twice, so no Undo of one exists; a row must not go for asking.""" + user = make_user() + _seed_videos(db, ["a", "b"]) + pl = _playlist(db, user, "L", ["a", "b"]) + + r = as_user(user).post( + f"/api/playlists/{pl.id}/items/undo-move", + json={"from_playlist_id": pl.id, "moved": ["a"], "positions": [0]}, + ) + + assert r.status_code == 400 + assert r.json()["detail"] == {"code": "playlist_move", "reason": "same_playlist"} + assert _ids(db, pl.id) == ["a", "b"] + + +@pytest.mark.parametrize( + ("path", "key", "extra"), + [ + ("items/restore", "video_ids", {}), + ("items/undo-move", "moved", {"from_playlist_id": None}), + ], +) +def test_two_videos_cannot_ask_for_one_place(db, make_user, as_user, path, key, extra): + user = make_user() + _seed_videos(db, ["a", "b", "c"]) + pl = _playlist(db, user, "L", ["c"]) + other = _playlist(db, user, "Other", ["a", "b"]) + body = {key: ["a", "b"], "positions": [0, 0]} + body.update({k: other.id for k in extra}) + + r = as_user(user).post(f"/api/playlists/{pl.id}/{path}", json=body) + + assert r.status_code == 422 + assert _ids(db, pl.id) == ["c"] + + def test_a_reorder_without_its_videos_is_refused(db, make_user, as_user): user = make_user() pl = _playlist(db, user, "L", []) diff --git a/backend/tests/test_plex_playlist_add_positions.py b/backend/tests/test_plex_playlist_add_positions.py index 04b1552b..4d4989ff 100644 --- a/backend/tests/test_plex_playlist_add_positions.py +++ b/backend/tests/test_plex_playlist_add_positions.py @@ -6,6 +6,8 @@ rows, since positions have gaps after a removal. One request: an append plus a s leaves the rows at the end whenever that call fails, and would overwrite another tab's changes. """ +import pytest + from app.plex.models import PlexItem, PlexLibrary, PlexPlaylist, PlexPlaylistItem @@ -101,6 +103,22 @@ def test_positions_must_pair_with_the_keys(db, make_user, as_user): assert _order(db, pl.id)[0] == ["a"] +@pytest.mark.parametrize( + ("method", "path"), + [("post", "items/bulk"), ("post", "items/remove-bulk"), ("put", "order")], +) +def test_a_body_without_its_keys_is_refused_rather_than_acting_on_none( + db, make_user, as_user, method, path +): + user = make_user() + pl = _seed(db, user, ["a"], extra=()) + + r = getattr(as_user(user), method)(f"/api/plex/playlists/{pl.id}/{path}", json={"keys": ["a"]}) + + assert r.status_code == 422 + assert _order(db, pl.id)[0] == ["a"] + + def test_remove_then_put_back_is_no_change(db, make_user, as_user): user = make_user() pl = _seed(db, user, ["a", "b", "c", "d"], extra=()) diff --git a/docs/DESIGN.md b/docs/DESIGN.md index a61d87b0..281e8f77 100644 --- a/docs/DESIGN.md +++ b/docs/DESIGN.md @@ -543,8 +543,9 @@ of a full-width panel and nothing on the right. 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. - An Undo brings back what is still there to bring: a video the move's target no longer holds stays where - it went, and a warning says how many came back. + An Undo brings back what is still there to bring: a video the move put in the target and the target no + longer holds stays where it went, and a warning says how many came back. One the target held already only + left the source, so it returns there. Delete takes the selection when there is one, else the row under the keyboard, with no confirm. With every tick hidden by a filter it removes nothing and says so. An Esc during a drag puts the drag down, and only that. Below `xl` the band's buttons are glyphs named in their tooltips, and a filter's hidden count is "+N" with @@ -860,4 +861,4 @@ of a full-width panel and nothing on the right. | 2026-11-01 | Playlist heading on a narrow window (owner A) | The list's name had no width left: below `xl` Unsynced is its dot (the word read out, the hint in the tooltip); on a phone the cover is gone and Play all is its glyph, named in the tooltip. | Every word kept, the list's name at no width | A heading without its name says nothing | | 2026-11-02 | Dragging onto a list (owner D5 a, D6 a) | The grip carries a row, or every selected row with a selected one, onto a list in the rail; the toast undoes. Many dropped inside the list stay; a coarse pointer's drawer never peeks, so its rail is a target only when pinned. | Only the grabbed row going along; the grip off under a view | One row moving out of three lit ones misleads, and a filter is how the rows to move are found | | 2026-11-02 | Delete with every tick hidden (owner a) | It removes nothing and a warning says the filter hides them | Taking the row under the keyboard | While the band shows a selection, Delete means that selection: a row no one ticked must not go | -| 2026-11-02 | Undo of a video gone further (owner a) | Only what the target still holds comes back, and a warning says how many | Everything back, whatever it did since | A move skips a video that left the source; the same rule back keeps a video out of lists the reader never put it in | +| 2026-11-02 | Undo of a video gone further (owner a) | A video the move put in the target comes back only if the target still holds it, and a warning says how many | Everything back, whatever it did since | A move skips a video that left the source; the same rule back keeps a video out of lists the reader never put it in | diff --git a/frontend/src/i18n/locales/en/playlists.json b/frontend/src/i18n/locales/en/playlists.json index 092fc3bd..56bf33a9 100644 --- a/frontend/src/i18n/locales/en/playlists.json +++ b/frontend/src/i18n/locales/en/playlists.json @@ -115,11 +115,14 @@ "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", + "moveNoneLeft_one": "Nothing moved: it isn’t in this playlist any more", + "moveNoneLeft_other": "Nothing moved: they aren’t in this playlist any more", + "removeNoneLeft_one": "Nothing removed: it isn’t in this playlist any more", + "removeNoneLeft_other": "Nothing removed: they aren’t in this playlist any more", "selectionCleared": "Selection cleared", "removeAllHidden": "The selected rows are hidden by the filter", - "undoNoneBack": "Nothing came back: those videos aren’t where they were any more", + "undoNoneBack_one": "Nothing came back: that video isn’t where it was any more", + "undoNoneBack_other": "Nothing came back: those videos aren’t where they were any more", "undoSomeBack_one": "Only {{count}} video came back: the rest aren’t where they were any more", "undoSomeBack_other": "Only {{count}} videos came back: the rest aren’t where they were any more", "dragCancelledMany_one": "Move cancelled; {{count}} video stays where it was", diff --git a/frontend/src/i18n/locales/hu/keys.json b/frontend/src/i18n/locales/hu/keys.json index e8e27173..8dbe3e0e 100644 --- a/frontend/src/i18n/locales/hu/keys.json +++ b/frontend/src/i18n/locales/hu/keys.json @@ -36,7 +36,7 @@ "gridMove": "Mozgás a listában", "open": "Az aktuális elem megnyitása", "listRemove": "Eltávolítás a listából", - "listSelect": "Az aktuális elem kijelölése, vagy a kijelölés törlése", + "listSelect": "Az aktuális elem kijelölése vagy a jelölés levétele", "listClearSelection": "A kijelölés törlése", "cap": { "space": "Szóköz", diff --git a/frontend/src/i18n/locales/hu/playlists.json b/frontend/src/i18n/locales/hu/playlists.json index f0e9daa7..ee10141f 100644 --- a/frontend/src/i18n/locales/hu/playlists.json +++ b/frontend/src/i18n/locales/hu/playlists.json @@ -115,11 +115,14 @@ "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", + "moveNoneLeft_one": "Nem történt áthelyezés: ez már nincs ebben a listában", + "moveNoneLeft_other": "Nem történt áthelyezés: ezek már nincsenek ebben a listában", + "removeNoneLeft_one": "Nem történt eltávolítás: ez már nincs ebben a listában", + "removeNoneLeft_other": "Nem történt eltávolítás: ezek már nincsenek ebben a listában", "selectionCleared": "Kijelölés törölve", "removeAllHidden": "A kijelölt sorokat a szűrő elrejti", - "undoNoneBack": "Semmi nem került vissza: ezek a videók már nincsenek ott, ahol voltak", + "undoNoneBack_one": "Semmi nem került vissza: ez a videó már nincs ott, ahol volt", + "undoNoneBack_other": "Semmi nem került vissza: ezek a videók már nincsenek ott, ahol voltak", "undoSomeBack_one": "Csak {{count}} videó került vissza: a többi már nincs ott, ahol volt", "undoSomeBack_other": "Csak {{count}} videó került vissza: a többi már nincs ott, ahol volt", "dragCancelledMany_one": "Áthelyezés megszakítva; {{count}} videó a helyén marad", diff --git a/frontend/src/lib/api/playlists.ts b/frontend/src/lib/api/playlists.ts index 81472078..4150d508 100644 --- a/frontend/src/lib/api/playlists.ts +++ b/frontend/src/lib/api/playlists.ts @@ -68,8 +68,6 @@ export const playlistsApi = { req(`/api/playlists/${id}`, { method: "PATCH", body: JSON.stringify({ name }) }), deletePlaylist: (id: number, onYoutube = false) => req(`/api/playlists/${id}${onYoutube ? "?on_youtube=true" : ""}`, { method: "DELETE" }), - // `position` (an index in the stored order) puts it back where a removal took it from — the - // list page's Undo; omitted, it is appended. addToPlaylist: (id: number, videoId: string) => req(`/api/playlists/${id}/items`, { method: "POST", diff --git a/frontend/src/modules/playlists/Playlists.dom.test.tsx b/frontend/src/modules/playlists/Playlists.dom.test.tsx index 95043c71..7d3ec084 100644 --- a/frontend/src/modules/playlists/Playlists.dom.test.tsx +++ b/frontend/src/modules/playlists/Playlists.dom.test.tsx @@ -795,7 +795,10 @@ describe("Playlists — removing a row, and undoing it", () => { act(() => lastToast().action!.onClick()); await waitFor(() => expect(h.notify).toHaveBeenCalledWith( - expect.objectContaining({ level: "warning", message: msg("playlists.undoNoneBack") }), + expect.objectContaining({ + level: "warning", + message: msg("playlists.undoNoneBack", { count: 1 }), + }), ), ); }); @@ -1761,7 +1764,7 @@ describe("Playlists — acting on the selected rows", () => { 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().message).toEqual(msg("playlists.moveNoneLeft", { count: 2 })); expect(toast().action).toBeUndefined(); }); @@ -2148,7 +2151,9 @@ describe("Playlists — acting on the selected rows", () => { mount(abc); await tick("Video A", "Video B"); fireEvent.click(await button("playlists.removeSelected")); - await waitFor(() => expect(toast().message).toEqual(msg("playlists.removeNoneLeft"))); + await waitFor(() => + expect(toast().message).toEqual(msg("playlists.removeNoneLeft", { count: 2 })), + ); expect(toast().action).toBeUndefined(); }); @@ -2168,7 +2173,7 @@ describe("Playlists — acting on the selected rows", () => { }); it.each([ - [0, msg("playlists.undoNoneBack")], + [0, msg("playlists.undoNoneBack", { count: 2 })], [1, msg("playlists.undoSomeBack", { count: 1 })], ])("says what came back when a move's Undo brought %i of 2", async (added, said) => { vi.spyOn(api, "moveManyToPlaylist").mockResolvedValue(moved(["A", "B"]) as never); @@ -2248,3 +2253,330 @@ describe("Playlists — acting on the selected rows", () => { expect(cell.classList.contains("@sm:table-cell")).toBe(true); }); }); + +describe("Playlists — writes that overlap", () => { + const later = () => { + let ok!: (v: T) => void; + let no!: (e: unknown) => void; + const promise = new Promise((a, b) => { + ok = a; + no = b; + }); + return { promise, ok, no }; + }; + const landed = () => act(async () => void (await new Promise((r) => setTimeout(r, 20)))); + const abc = () => ({ 1: [vid("A", 0), vid("B", 0), vid("C", 0)] }); + const inPage = () => screen.findByRole("button", { name: "card.play — Video A" }); + const reads = () => vi.mocked(api.playlist).mock.calls.length; + const undoOrder = () => screen.getByRole("button", { name: "common.undo" }) as HTMLButtonElement; + const lastUndo = () => + h.notify.mock.calls + .map(([n]) => n as { action?: { onClick: () => void } }) + .filter((n) => n.action) + .pop(); + // A menu row acts after the menu has closed, so a test that goes on at once acts before it. + const removeRow = async (title: string, ...left: string[]) => { + await rowMenu(title, "playlists.removeItem"); + await waitFor(() => expect(rowsOnScreen()).toEqual(left)); + }; + + it("keeps a row out when a read sent before its removal lands after it, and its history too", async () => { + vi.spyOn(api, "reorderPlaylist").mockResolvedValue(undefined as never); + const second = later(); + vi.spyOn(api, "removeManyFromPlaylist") + .mockResolvedValueOnce(wentOut("A") as never) + .mockReturnValueOnce(second.promise as never); + mount({ 1: [...abc()[1], vid("D", 0)] }); + await inPage(); + await rowMenu("Video C", "common.order.up"); + await waitFor(() => expect(undoOrder().disabled).toBe(false)); + await landed(); + const read = later(); + vi.mocked(api.playlist).mockReturnValue(read.promise as never); + const before = reads(); + await removeRow("Video A", "Video C", "Video B", "Video D"); + await waitFor(() => expect(reads()).toBeGreaterThan(before)); + await removeRow("Video B", "Video C", "Video D"); + await act(async () => read.ok(list(1, [vid("C", 0), vid("B", 0), vid("D", 0)]))); + await landed(); + expect(rowsOnScreen()).toEqual(["Video C", "Video D"]); + expect(undoOrder().disabled).toBe(false); + // …and the cache too, which is what a failed write goes back to. + expect(qc.getQueryData<{ items: Video[] }>(qk.playlist(1))?.items.map((v) => v.id)).toEqual([ + "C", + "D", + ]); + }); + + it("reads the list again only when the last write is in", async () => { + const first = later(); + const second = later(); + vi.spyOn(api, "removeManyFromPlaylist") + .mockReturnValueOnce(first.promise as never) + .mockReturnValueOnce(second.promise as never); + mount(abc()); + await inPage(); + await landed(); + await removeRow("Video A", "Video B", "Video C"); + await removeRow("Video B", "Video C"); + const before = reads(); + await act(async () => first.ok(wentOut("A"))); + await landed(); + expect(reads()).toBe(before); + await act(async () => second.ok(wentOut("B"))); + await waitFor(() => expect(reads()).toBeGreaterThan(before)); + }); + + it("takes no row back from a read that lands while a write is out, and takes the next one", async () => { + const write = later(); + vi.spyOn(api, "removeManyFromPlaylist").mockReturnValue(write.promise as never); + const lists = abc(); + mount(lists); + await inPage(); + await removeRow("Video A", "Video B", "Video C"); + await act(async () => { + await qc.refetchQueries({ queryKey: qk.playlist(1) }); + }); + await landed(); + expect(rowsOnScreen()).toEqual(["Video B", "Video C"]); + lists[1] = [vid("B", 0), vid("C", 0), vid("X", 0)]; + await act(async () => write.ok(wentOut("A"))); + await waitFor(() => expect(rowsOnScreen()).toEqual(["Video B", "Video C", "Video X"])); + }); + + it("seeds a list picked while a write to it is out", async () => { + const write = later(); + vi.spyOn(api, "removeManyFromPlaylist").mockReturnValue(write.promise as never); + const { rerender } = mount({ 1: [vid("A", 0)], 2: [vid("B", 0), vid("C", 0)] }); + await play("Video A"); + await waitFor(() => expect(h.player).not.toBeNull()); + h.selectedId = 2; + rerender(); + await screen.findByRole("button", { name: "card.play — Video C" }); + act(() => void h.player!.onRemoveFromList!(vid("A", 0))); + h.selectedId = 1; + rerender(); + await landed(); + expect(screen.queryByRole("button", { name: "card.play — Video C" })).toBeNull(); + await act(async () => write.ok(wentOut("A"))); + }); + + it.each([ + ["the first", 0, 1], + ["the second", 1, 0], + ])( + "puts both rows back in their places when %s of two failures comes first", + async (_n, x, y) => { + const writes = [later(), later()]; + vi.spyOn(api, "removeManyFromPlaylist") + .mockReturnValueOnce(writes[0]!.promise as never) + .mockReturnValueOnce(writes[1]!.promise as never); + mount(abc()); + await inPage(); + await landed(); + vi.mocked(api.playlist).mockRejectedValue(new TypeError("Failed to fetch")); + await removeRow("Video A", "Video B", "Video C"); + await removeRow("Video B", "Video C"); + await act(async () => writes[x]!.no(new TypeError("Failed to fetch"))); + await landed(); + await act(async () => writes[y]!.no(new TypeError("Failed to fetch"))); + await landed(); + expect(rowsOnScreen()).toEqual(["Video A", "Video B", "Video C"]); + }, + ); + + it("puts back only the row whose write failed, not one another write took out", async () => { + const first = later(); + const second = later(); + vi.spyOn(api, "removeManyFromPlaylist") + .mockReturnValueOnce(first.promise as never) + .mockReturnValueOnce(second.promise as never); + mount(abc()); + await inPage(); + await landed(); + vi.mocked(api.playlist).mockRejectedValue(new TypeError("Failed to fetch")); + await removeRow("Video A", "Video B", "Video C"); + await removeRow("Video B", "Video C"); + await act(async () => second.ok(wentOut("B"))); + await act(async () => first.no(new TypeError("Failed to fetch"))); + await landed(); + expect(rowsOnScreen()).toEqual(["Video A", "Video C"]); + }); + + it("brings back no row taken out while a reorder was on its way", async () => { + const put = later(); + vi.spyOn(api, "reorderPlaylist").mockReturnValue(put.promise as never); + vi.spyOn(api, "removeManyFromPlaylist").mockResolvedValue(wentOut("B") as never); + mount(abc()); + await inPage(); + await landed(); + await rowMenu("Video C", "common.order.up"); + await waitFor(() => expect(api.reorderPlaylist).toHaveBeenCalled()); + vi.mocked(api.playlist).mockReturnValue(new Promise(() => {}) as never); + await removeRow("Video B", "Video A", "Video C"); + await act(async () => put.ok(undefined)); + await landed(); + expect(rowsOnScreen()).toEqual(["Video A", "Video C"]); + expect(undoOrder().disabled).toBe(false); + }); + + describe("from the player, on a list the cache has dropped", () => { + const dropped = async () => { + const lists: Record = { + 1: [vid("U", 0), vid("V", 0), vid("W", 0)], + 2: [vid("Y", 0)], + }; + const { rerender } = mount(lists); + await play("Video U"); + await waitFor(() => expect(h.player).not.toBeNull()); + h.selectedId = 2; + rerender(); + await screen.findByRole("button", { name: "card.play — Video Y" }); + qc.removeQueries({ queryKey: qk.playlist(1), exact: true }); + return lists; + }; + + it("offers an Undo that puts the video back at its place", async () => { + const lists = await dropped(); + vi.spyOn(api, "removeManyFromPlaylist").mockImplementation((async () => { + lists[1] = lists[1]!.filter((v) => v.id !== "V"); + return wentOut("V"); + }) as never); + const restore = vi + .spyOn(api, "restoreToPlaylist") + .mockResolvedValue({ ok: true, added: 1 } as never); + await act(async () => { + await h.player!.onRemoveFromList!(vid("V", 0)); + }); + await act(async () => lastUndo()!.action!.onClick()); + await waitFor(() => expect(restore).toHaveBeenCalledWith(1, ["V"], [1])); + }); + + it("removes nothing and says so when the list cannot be read first", async () => { + await dropped(); + const remove = vi.spyOn(api, "removeManyFromPlaylist"); + vi.mocked(api.playlist).mockRejectedValue(new TypeError("Failed to fetch")); + let done: boolean | undefined; + await act(async () => { + done = await h.player!.onRemoveFromList!(vid("V", 0)); + }); + expect(done).toBe(false); + expect(remove).not.toHaveBeenCalled(); + expect(h.notify).toHaveBeenCalledWith(expect.objectContaining({ level: "error" })); + }); + }); + + it("refreshes what Add to playlist knows about every row the writes took", async () => { + const first = later(); + const second = later(); + vi.spyOn(api, "removeManyFromPlaylist") + .mockReturnValueOnce(first.promise as never) + .mockReturnValueOnce(second.promise as never); + mount(abc()); + await inPage(); + await landed(); + qc.setQueryData(qk.playlistMembership("A"), []); + await removeRow("Video A", "Video B", "Video C"); + await removeRow("Video B", "Video C"); + await act(async () => first.ok(wentOut("A"))); + await act(async () => second.ok(wentOut("B"))); + await landed(); + expect(qc.getQueryState(qk.playlistMembership("A"))?.isInvalidated).toBe(true); + }); + + it("places an Undo by what the server says, though a removal starts while it asks", async () => { + const restore = vi + .spyOn(api, "restoreToPlaylist") + .mockResolvedValue({ ok: true, added: 1 } as never); + const lists = abc(); + const remove = vi.spyOn(api, "removeManyFromPlaylist"); + remove.mockImplementationOnce((async () => { + lists[1] = lists[1].filter((v) => v.id !== "B"); + return wentOut("B"); + }) as never); + mount(lists); + await inPage(); + await landed(); + await removeRow("Video B", "Video A", "Video C"); + await waitFor(() => expect(lastUndo()).toBeTruthy()); + await landed(); + const read = later(); + vi.mocked(api.playlist).mockReturnValue(read.promise as never); + await act(async () => lastUndo()!.action!.onClick()); + remove.mockReturnValueOnce(new Promise(() => {}) as never); + await removeRow("Video C", "Video A"); + // Another tab has put a row at the top meanwhile: only a fresh answer knows it. + await act(async () => read.ok(list(1, [vid("X", 0), vid("A", 0), vid("C", 0)]))); + await waitFor(() => expect(restore).toHaveBeenCalledWith(1, ["B"], [2])); + }); + + it("speaks of one video when one row found nothing to do", async () => { + vi.spyOn(api, "removeManyFromPlaylist").mockResolvedValue(wentOut() as never); + mount(abc()); + await inPage(); + await rowMenu("Video A", "playlists.removeItem"); + await waitFor(() => + expect(h.notify).toHaveBeenCalledWith( + expect.objectContaining({ message: msg("playlists.removeNoneLeft", { count: 1 }) }), + ), + ); + }); +}); + +describe("Playlists — the focus when what holds it goes", () => { + const box = (name: string) => screen.findByRole("checkbox", { name }); + const headerBox = () => screen.getByRole("checkbox", { name: "playlists.selectAllShown" }); + const press = (key: string) => + fireEvent.keyDown(document.activeElement ?? document.body, { key, bubbles: true }); + + it("takes the header's box when the last row a filter shows goes, the list not empty", async () => { + vi.spyOn(api, "removeManyFromPlaylist").mockResolvedValue(wentOut("B") as never); + window.history.replaceState( + null, + "", + `/?page=playlists&pl=1&plf=${encodeURIComponent(JSON.stringify({ title: "Video B" }))}`, + ); + mount({ 1: [vid("A", 0), vid("B", 0), vid("C", 0)] }); + const row = await box("playlists.selectRow:Video B"); + fireEvent.click(row); + row.focus(); + await act(async () => press("Delete")); + await waitFor(() => expect(api.removeManyFromPlaylist).toHaveBeenCalled()); + await waitFor(() => expect(document.activeElement).toBe(headerBox())); + }); + + it("takes the header's box when Save this order goes with the view", async () => { + vi.spyOn(api, "reorderPlaylist").mockResolvedValue(undefined as never); + window.history.replaceState(null, "", "/?page=playlists&pl=1&plsort=title:desc"); + mount({ 1: [vid("A", 0), vid("B", 0)] }); + const save = await screen.findByRole("button", { name: "playlists.saveOrder" }); + save.focus(); + fireEvent.click(save); + await waitFor(() => + expect(screen.queryByRole("button", { name: "playlists.saveOrder" })).toBeNull(), + ); + expect(document.activeElement).toBe(headerBox()); + }); + + it("takes the header's box when grouping goes off on a list too short to offer it", async () => { + window.history.replaceState(null, "", "/?page=playlists&pl=1&plgroup=1"); + mount({ 1: [vid("A", 0)] }); + const group = await box("playlists.groupByChannel"); + group.focus(); + fireEvent.click(group); + await waitFor(() => + expect(screen.queryByRole("checkbox", { name: "playlists.groupByChannel" })).toBeNull(), + ); + expect(document.activeElement).toBe(headerBox()); + }); + + it("leaves the focus on the grouping box when the row stays", async () => { + window.history.replaceState(null, "", "/?page=playlists&pl=1&plgroup=1"); + mount({ 1: [vid("A", 0), vid("B", 0)] }); + const group = await box("playlists.groupByChannel"); + group.focus(); + fireEvent.click(group); + await waitFor(() => expect((group as HTMLInputElement).checked).toBe(false)); + expect(document.activeElement).toBe(group); + }); +}); diff --git a/frontend/src/modules/playlists/Playlists.tsx b/frontend/src/modules/playlists/Playlists.tsx index 6a3a5ac3..d02a1e2d 100644 --- a/frontend/src/modules/playlists/Playlists.tsx +++ b/frontend/src/modules/playlists/Playlists.tsx @@ -10,7 +10,7 @@ import { } from "react"; import { useLatest, useStableCallback } from "../../lib/useLatest"; import { useTranslation } from "react-i18next"; -import { useQuery, useQueryClient } from "@tanstack/react-query"; +import { type QueryClient, useQuery, useQueryClient } from "@tanstack/react-query"; import { qk } from "../../lib/queryKeys"; import { hasResumePoint } from "../../lib/api/feed"; import { @@ -271,6 +271,30 @@ export default function Playlists() { ); } +/** `old` with `ids` back at their places, from the list as it stood before they went. Only these rows: + * another write out at the same time keeps what it did. */ +function withRowsBack(old: PlaylistDetail, before: PlaylistDetail, ids: readonly string[]) { + const back = reinsertAt( + before.items.map((v) => v.id), + ids, + old.items.map((v) => v.id), + ); + const rows = new Map(before.items.map((v) => [v.id, v])); + const items = [...old.items]; + back.keys.forEach((id, i) => items.splice(back.positions[i]!, 0, rows.get(id)!)); + return { ...old, items }; +} + +/** Reads a list into the cache. Says what went wrong, if anything did. */ +async function readFailure(qc: QueryClient, listId: number): Promise { + try { + await qc.fetchQuery({ queryKey: qk.playlist(listId), queryFn: () => api.playlist(listId) }); + return null; + } catch (e) { + return e; + } +} + function PlaylistDetail() { // The selected playlist lives in PlaylistsProvider, shared with the PlaylistsRail (which owns the // list + its rail controls). This detail pane reacts to the selection. @@ -282,6 +306,12 @@ function PlaylistDetail() { const [renaming, setRenaming] = useState(false); const { target: renameTarget, arm: armRenameReturn } = useReturnFocus(renaming); const [renameValue, setRenameValue] = useState(""); + // The rows leaving each list right now, and the list as it stood when the first of them left. A read + // can answer from before a write that is still out, so while one is out the list is not read again + // and what a read brings is not taken on screen. The list as it stood is for all of those writes: + // the later ones' own view of it lacks the rows the earlier ones took, and so their neighbours. + const writesOut = useRef(new Map()); + const refreshDue = useRef(new Set()); // The stored order is undoable: drag and "save this order" both go through `order.set`, so each // is reversible (buttons + Ctrl+Z/Y). onApply persists the new order to the server. const order = useUndoable([], { @@ -299,14 +329,16 @@ function PlaylistDetail() { // from before this reorder — so a SECOND reorder failing in that window rolled the rows // back to an order the server does not have, permanently (the membership guard cannot // correct an order). One accepted write, one cache entry that matches it. - qc.setQueryData(qk.playlist(listId), (old) => - old ? { ...old, items: next } : old, - ); + // A row taken out while this was on its way stays out: `next` is from before that. + qc.setQueryData(qk.playlist(listId), (old) => { + if (!old) return old; + const held = new Set(old.items.map((v) => v.id)); + return { ...old, items: next.filter((v) => held.has(v.id)) }; + }); // Refresh the sidebar (cover may change) and the detail (so the now-dirty state, // and the Reset/Unsynced indicators, show without an F5). The membership guard // keeps the refetch from wiping the undo history on a pure reorder. - qc.invalidateQueries({ queryKey: qk.playlists() }); - qc.invalidateQueries({ queryKey: qk.playlist(listId) }); + refreshAfterEdit([]); }) .catch((e) => { // Was a bare .then(): a failed reorder left the UI showing an order the server had @@ -314,7 +346,7 @@ function PlaylistDetail() { // still has — or the rejected one becomes the base for the next drag. notifyYouTubeActionError(e, msg("playlists.reorderFailed")); restoreFromCache(listId); - qc.invalidateQueries({ queryKey: qk.playlist(listId) }); + refreshAfterEdit([]); }); }, }); @@ -414,6 +446,13 @@ function PlaylistDetail() { const { reset: resetOrder, rebase: rebaseOrder } = order; useEffect(() => { if (!detail) return; + // A write out on the list these rows are from: this may be a read from before it landed, and the + // refresh after the last write brings what is true. A list only just picked has no rows yet. + if ( + writesOut.current.has(selectedId as number) && + lastSetRef.current.startsWith(`${selectedId}:`) + ) + return; // Scoped to the playlist: two lists holding the SAME videos would otherwise look identical to // the guard, so the second would render in the first's order and inherit its undo stack. const key = `${selectedId}:${idsKey(detail.items)}`; @@ -591,15 +630,25 @@ function PlaylistDetail() { } // From a button that goes with the view: a focus on it would fall to , and the next Tab start // over at the top of the page. - const resetView = (e: MouseEvent) => { + const leaveView = (e: MouseEvent) => { if (document.activeElement === e.currentTarget) (selectAllRef.current ?? renameTarget.current)?.focus(); + }; + const resetView = (e: MouseEvent) => { + leaveView(e); clearView(); }; function saveOrder() { order.set(shown); // onApply persists it clearView(); } + // The grouping row is only there for a list long enough to group, or a view to take off: with this + // the last of the view on a short list, it goes too. + function setGrouping(on: boolean) { + if (!on && items.length <= 1 && isStoredSort(viewSort) && !filterActive) + (selectAllRef.current ?? renameTarget.current)?.focus(); + setGroupBy(on); + } const sensors = useSensors( useSensor(PointerSensor, { activationConstraint: { distance: 4 } }), @@ -731,36 +780,62 @@ function PlaylistDetail() { 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)}`; - // The table goes with its last row: a focus in it or in the band would fall to , and the - // next Tab start over at the top of the page. The list's "…" takes it. + // The last row shown goes: a focus in the table or in the band would fall to , and the next + // Tab start over at the top of the page. The header's box takes it, while a filter hides rows and + // so keeps the table; else the table goes too, and the list's "…" does. const held = document.activeElement; - if (!next.length && (tableRef.current?.contains(held) || bandRef.current?.contains(held))) - renameTarget.current?.focus(); + if ( + !shown.some((v) => !gone.has(v.id)) && + (tableRef.current?.contains(held) || bandRef.current?.contains(held)) + ) + (next.length ? selectAllRef.current : renameTarget.current)?.focus(); } // …and out of the CACHED list too, not just the rows. Anything that rewrites that cache entry // before the refetch lands handed the membership guard a list that still held the video — the // player's own progress checkpoint does exactly that when a removal steps it to the next item — // so the guard reset the order: the removed row came back and the undo history the rebase had - // kept went with it. (A failure puts it back: `putBack`.) - const before = qc.getQueryData(qk.playlist(listId)); + // kept went with it. (A failure puts it back: `putBack`.) A read on its way would do the same when + // it lands, so it is called off; the write's own refresh replaces it. + const out = writesOut.current.get(listId); + const before = out ? out.before : qc.getQueryData(qk.playlist(listId)); + writesOut.current.set(listId, { count: (out?.count ?? 0) + 1, before }); + void qc.cancelQueries({ queryKey: qk.playlist(listId) }); qc.setQueryData(qk.playlist(listId), (old) => old ? { ...old, items: old.items.filter((v) => !gone.has(v.id)) } : old, ); return before; } + function writeDone(listId: number) { + const out = writesOut.current.get(listId); + if (out && out.count > 1) out.count--; + else writesOut.current.delete(listId); + } // 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); + function putBack( + listId: number, + before: PlaylistDetail | undefined, + ids: readonly string[], + ticked: readonly string[], + ) { + if (before) + qc.setQueryData(qk.playlist(listId), (old) => + old ? withRowsBack(old, before, ids) : 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. + // key, so it cannot cover the target: the whole `["playlist"]` prefix, as AddToPlaylist does. The + // last write out does it for them all (see `writesOut`). function refreshAfterEdit(ids: readonly string[]) { + for (const id of ids) refreshDue.current.add(id); + if (writesOut.current.size) return; qc.invalidateQueries({ queryKey: qk.playlists() }); qc.invalidateQueries({ queryKey: qk.playlist() }); - for (const id of ids) qc.invalidateQueries({ queryKey: qk.playlistMembership(id) }); + for (const id of refreshDue.current) + qc.invalidateQueries({ queryKey: qk.playlistMembership(id) }); + refreshDue.current.clear(); } const titleOf = (rows: Video[], id: string | undefined) => raw(rows.find((v) => v.id === id)?.title ?? ""); @@ -768,21 +843,16 @@ function PlaylistDetail() { const tickOf = (v: Video) => (selection.isSelected(v.id) ? [v.id] : []); // Where rows go back when their Undo comes: against the list as it is NOW, fetched fresh, and after // their neighbours in the STORED order they stood in, so neither a view sort on screen nor a change - // in between shifts them (`reinsertAt`). + // in between shifts them (`reinsertAt`). Past the cache: a removal starting meanwhile calls off the + // cache's read, which then answers with what the cache holds, not with what is fresh. const placesNow = (listId: number, storedBefore: string[], ids: string[]) => - qc - .fetchQuery({ - queryKey: qk.playlist(listId), - queryFn: () => api.playlist(listId), - staleTime: 0, - }) - .then((now) => - reinsertAt( - storedBefore, - ids, - now.items.map((v) => v.id), - ), - ); + api.playlist(listId).then((now) => + reinsertAt( + storedBefore, + ids, + now.items.map((v) => v.id), + ), + ); // An Undo that could not bring everything back says so: the server leaves out a video it cannot put // back (gone on from the target, or from the catalog), and silence would read as all of them. function sayWhatCameBack(asked: number, added: number) { @@ -791,7 +861,7 @@ function PlaylistDetail() { level: "warning", message: added ? msg("playlists.undoSomeBack", { count: added }) - : msg("playlists.undoNoneBack"), + : msg("playlists.undoNoneBack", { count: asked }), }); } @@ -812,8 +882,9 @@ function PlaylistDetail() { // fault `req` already put in its error dialog is not toasted again (`notifyActionError` checks). notifyActionError(e, failTitle); } + writeDone(listId); if (!done) { - putBack(listId, before, ticked); + putBack(listId, before, ids, ticked); refreshAfterEdit(ids); return; } @@ -824,7 +895,7 @@ function PlaylistDetail() { // to failing again. setLastMoveTargetId(target.id); if (!left.length) { - notify({ level: "warning", message: msg("playlists.moveNoneLeft") }); + notify({ level: "warning", message: msg("playlists.moveNoneLeft", { count: rows.length }) }); return; } const name = playlistNameText(target); @@ -837,8 +908,8 @@ function PlaylistDetail() { : msg("playlists.movedManyToast", { count: left.length, name }), action: { label: msg("common.undo"), - // A video back in the source by then stays where it is, in both lists. One gone on from the - // target stays out: the server says how many came back. + // A video back in the source by then stays where it is, in both lists. One this move put in the + // target and gone on from there stays out: the server says how many came back. onClick: () => void placesNow(listId, storedBefore, left) .then((back) => { @@ -879,10 +950,19 @@ function PlaylistDetail() { if (listId == null || !rows.length) return false; 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", ); + // The open player can be on a list nothing has kept in the cache since the selection moved off it: + // its Undo needs the places, so without them nothing goes. + if (!qc.getQueryData(qk.playlist(listId))) { + const failure = await readFailure(qc, listId); + if (failure) { + notifyActionError(failure, failTitle); + return false; + } + } + const before = takeOut(listId, ids); let removed: string[] | null = null; try { removed = (await removePlaylistItems(qc, list, ids)).removed; @@ -892,17 +972,19 @@ function PlaylistDetail() { // `notifyActionError` checks. notifyActionError(e, failTitle); } + writeDone(listId); + // …and the rows go back NOW, not when the resync lands: a removal that failed because the server + // was unreachable has a resync that fails too. + if (removed == null) putBack(listId, before, ids, ticked); refreshAfterEdit(ids); - if (removed == null) { - // …and the rows go back NOW, not when the resync lands: a removal that failed because the server - // was unreachable has a resync that fails too. - putBack(listId, before, ticked); - return false; - } + if (removed == null) return false; // A video someone else took out meanwhile is not this removal's to bring back. const went = removed; if (!went.length) { - notify({ level: "warning", message: msg("playlists.removeNoneLeft") }); + notify({ + level: "warning", + message: msg("playlists.removeNoneLeft", { count: rows.length }), + }); return true; } const storedBefore = before?.items.map((v) => v.id) ?? []; @@ -1439,7 +1521,7 @@ function PlaylistDetail() { {t("playlists.groupByChannel")} @@ -1457,7 +1539,10 @@ function PlaylistDetail() {