Merge chore/code-hygiene: Phase 2 #4 Playlists (cleanup + bug fixes)

Cleanup (_remove_item dedup; drop dead Check import + plName shadow) plus
review-found bug fixes: reorder position-collision under concurrent edits,
push/sync/revert YouTube 403s now route to the Connect affordance, and
removeItem rolls back on failure. Verified: ruff, tsc, localdev boots healthy.
This commit is contained in:
npeter83 2026-07-11 18:33:28 +02:00
commit a84cb991e7
2 changed files with 46 additions and 31 deletions

View file

@ -74,6 +74,23 @@ def _add_item(db: Session, pl: Playlist, video_id: str, *, mark_dirty: bool) ->
return True return True
def _remove_item(db: Session, pl: Playlist, video_id: str, *, mark_dirty: bool) -> bool:
"""Remove a video from a playlist if present (idempotent). Returns whether it was removed.
`mark_dirty` recomputes the YouTube-sync dirty flag (skip for Watch later)."""
item = db.scalar(
select(PlaylistItem).where(
PlaylistItem.playlist_id == pl.id, PlaylistItem.video_id == video_id
)
)
if item is None:
return False
db.delete(item)
if mark_dirty:
recompute_dirty(db, pl)
db.commit()
return True
def _summary( def _summary(
db: Session, db: Session,
pl: Playlist, pl: Playlist,
@ -239,14 +256,7 @@ def remove_watch_later(
) )
) )
if pl is not None: if pl is not None:
item = db.scalar( _remove_item(db, pl, video_id, mark_dirty=False)
select(PlaylistItem).where(
PlaylistItem.playlist_id == pl.id, PlaylistItem.video_id == video_id
)
)
if item is not None:
db.delete(item)
db.commit()
return {"saved": False} return {"saved": False}
@ -452,15 +462,7 @@ def remove_item(
db: Session = Depends(get_db), db: Session = Depends(get_db),
) -> dict: ) -> dict:
pl = _own_playlist(db, user, playlist_id) pl = _own_playlist(db, user, playlist_id)
item = db.scalar( _remove_item(db, pl, video_id, mark_dirty=True)
select(PlaylistItem).where(
PlaylistItem.playlist_id == pl.id, PlaylistItem.video_id == video_id
)
)
if item is not None:
db.delete(item)
recompute_dirty(db, pl)
db.commit()
return {"playlist_id": pl.id, "video_id": video_id} return {"playlist_id": pl.id, "video_id": video_id}
@ -480,11 +482,20 @@ def reorder_items(
).scalars() ).scalars()
} }
pos = 0 pos = 0
seen: set[str] = set()
for vid in order: for vid in order:
it = items.get(vid) it = items.get(vid)
if it is not None: if it is not None:
it.position = pos it.position = pos
pos += 1 pos += 1
seen.add(vid)
# Any items the payload omitted (e.g. added concurrently in another tab) keep their relative
# order but get fresh trailing positions, so no two items collide on the same position.
for it in sorted(
(it for vid, it in items.items() if vid not in seen), key=lambda i: i.position
):
it.position = pos
pos += 1
recompute_dirty(db, pl) recompute_dirty(db, pl)
db.commit() db.commit()
return {"playlist_id": pl.id, "count": pos} return {"playlist_id": pl.id, "count": pos}

View file

@ -19,7 +19,6 @@ import { CSS } from "@dnd-kit/utilities";
import { import {
ArrowDown, ArrowDown,
ArrowUp, ArrowUp,
Check,
ExternalLink, ExternalLink,
GripVertical, GripVertical,
ListPlus, ListPlus,
@ -37,6 +36,7 @@ import {
import { api, type Playlist, type Video } from "../lib/api"; import { api, type Playlist, type Video } from "../lib/api";
import { formatDuration } from "../lib/format"; import { formatDuration } from "../lib/format";
import { notify } from "../lib/notifications"; import { notify } from "../lib/notifications";
import { notifyYouTubeActionError } from "../lib/youtubeErrors";
import { getAccountRaw, LS, readAccountMerged, setAccountRaw, writeAccount } from "../lib/storage"; import { getAccountRaw, LS, readAccountMerged, setAccountRaw, writeAccount } from "../lib/storage";
import { useUndoable } from "../lib/useUndoable"; import { useUndoable } from "../lib/useUndoable";
const PlayerModal = lazy(() => import("./PlayerModal")); const PlayerModal = lazy(() => import("./PlayerModal"));
@ -347,14 +347,13 @@ export default function Playlists({ canWrite }: { canWrite: boolean }) {
danger: true, danger: true,
}); });
if (!ok) return; if (!ok) return;
const plName = detail.kind === "watch_later" ? t("playlists.watchLater") : detail.name;
setReverting(true); setReverting(true);
try { try {
await api.revertPlaylist(selectedId); await api.revertPlaylist(selectedId);
refreshAll(); refreshAll();
notify({ level: "success", message: t("playlists.revertDone", { name: plName }) }); notify({ level: "success", message: t("playlists.revertDone", { name: plName(detail) }) });
} catch { } catch (e) {
notify({ level: "warning", message: t("playlists.revertFailed") }); notifyYouTubeActionError(e, t("playlists.revertFailed"));
} finally { } finally {
setReverting(false); setReverting(false);
} }
@ -362,12 +361,11 @@ export default function Playlists({ canWrite }: { canWrite: boolean }) {
async function pushToYoutube() { async function pushToYoutube() {
if (selectedId == null || !detail || pushing) return; if (selectedId == null || !detail || pushing) return;
const plName = detail.kind === "watch_later" ? t("playlists.watchLater") : detail.name;
setPushing(true); setPushing(true);
try { try {
const plan = await api.playlistPushPlan(selectedId); const plan = await api.playlistPushPlan(selectedId);
if (plan.action === "update" && !plan.to_insert && !plan.to_delete && !plan.to_reorder) { if (plan.action === "update" && !plan.to_insert && !plan.to_delete && !plan.to_reorder) {
notify({ level: "info", message: t("playlists.pushUpToDate", { name: plName }) }); notify({ level: "info", message: t("playlists.pushUpToDate", { name: plName(detail) }) });
return; return;
} }
if (!plan.affordable) { if (!plan.affordable) {
@ -406,10 +404,10 @@ export default function Playlists({ canWrite }: { canWrite: boolean }) {
if (r.failures.length) { if (r.failures.length) {
notify({ level: "warning", message: t("playlists.pushPartial", { count: r.failures.length }) }); notify({ level: "warning", message: t("playlists.pushPartial", { count: r.failures.length }) });
} else { } else {
notify({ level: "success", message: t("playlists.pushDone", { name: plName }) }); notify({ level: "success", message: t("playlists.pushDone", { name: plName(detail) }) });
} }
} catch { } catch (e) {
notify({ level: "warning", message: t("playlists.pushFailed") }); notifyYouTubeActionError(e, t("playlists.pushFailed"));
} finally { } finally {
setPushing(false); setPushing(false);
} }
@ -423,8 +421,8 @@ export default function Playlists({ canWrite }: { canWrite: boolean }) {
qc.invalidateQueries({ queryKey: ["playlists"] }); qc.invalidateQueries({ queryKey: ["playlists"] });
qc.invalidateQueries({ queryKey: ["playlist"] }); qc.invalidateQueries({ queryKey: ["playlist"] });
notify({ level: "success", message: t("playlists.syncedToast", { count: r.synced }) }); notify({ level: "success", message: t("playlists.syncedToast", { count: r.synced }) });
} catch { } catch (e) {
notify({ level: "warning", message: t("playlists.syncFailed") }); notifyYouTubeActionError(e, t("playlists.syncFailed"));
} finally { } finally {
setSyncing(false); setSyncing(false);
} }
@ -463,8 +461,14 @@ export default function Playlists({ canWrite }: { canWrite: boolean }) {
const next = items.filter((v) => v.id !== videoId); const next = items.filter((v) => v.id !== videoId);
order.reset(next); order.reset(next);
lastSetRef.current = idsKey(next); lastSetRef.current = idsKey(next);
await api.removeFromPlaylist(selectedId, videoId); try {
refreshAll(); await api.removeFromPlaylist(selectedId, videoId);
refreshAll();
} catch {
// The optimistic removal above didn't stick — resync from the server to restore the row.
// api.req already surfaced the error to the user via the global error dialog.
refreshAll();
}
} }
async function saveRename() { async function saveRename() {