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.
This commit is contained in:
@@ -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<unknown> {
|
||||
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<number, { count: number; before?: PlaylistDetail }>());
|
||||
const refreshDue = useRef(new Set<string>());
|
||||
// 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<Video[]>([], {
|
||||
@@ -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<PlaylistDetail>(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<PlaylistDetail>(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 <body>, and the next Tab start
|
||||
// over at the top of the page.
|
||||
const resetView = (e: MouseEvent<HTMLButtonElement>) => {
|
||||
const leaveView = (e: MouseEvent<HTMLButtonElement>) => {
|
||||
if (document.activeElement === e.currentTarget)
|
||||
(selectAllRef.current ?? renameTarget.current)?.focus();
|
||||
};
|
||||
const resetView = (e: MouseEvent<HTMLButtonElement>) => {
|
||||
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 <body>, 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 <body>, 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<PlaylistDetail>(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<PlaylistDetail>(qk.playlist(listId));
|
||||
writesOut.current.set(listId, { count: (out?.count ?? 0) + 1, before });
|
||||
void qc.cancelQueries({ queryKey: qk.playlist(listId) });
|
||||
qc.setQueryData<PlaylistDetail>(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<PlaylistDetail>(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() {
|
||||
</span>
|
||||
<CheckRow
|
||||
checked={groupBy}
|
||||
onChange={setGroupBy}
|
||||
onChange={setGrouping}
|
||||
className="text-xs text-muted"
|
||||
>
|
||||
{t("playlists.groupByChannel")}
|
||||
@@ -1457,7 +1539,10 @@ function PlaylistDetail() {
|
||||
<Button
|
||||
variant="outline-accent"
|
||||
size="xs"
|
||||
onClick={saveOrder}
|
||||
onClick={(e) => {
|
||||
leaveView(e);
|
||||
saveOrder();
|
||||
}}
|
||||
icon={<Check className="w-3.5 h-3.5" />}
|
||||
>
|
||||
{t("playlists.saveOrder")}
|
||||
|
||||
Reference in New Issue
Block a user