From 5fc37e46e4da49f5efa24048f635fc31e06e6db5 Mon Sep 17 00:00:00 2001 From: Myron Blair Date: Sun, 26 Jul 2026 22:24:09 -0500 Subject: [PATCH] Fix 8 code-review findings across transcode queue, requests, cleanup, music MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit - Episode transcode (single + bulk) now accepts storage_type=scan, matching stream_episode and the movie-transcode endpoint — Library-Scan-imported TV episodes can finally be transcoded, not just streamed. - retry_job now forwards content_type, fixing retry for failed/cancelled episode jobs (was silently treating them as movies and failing again). - cancel_job now updates the correct collection (movies vs episodes) based on content_type instead of always writing to db.movies. - approve_request atomically claims the request (pending -> approving) before contacting Radarr/Sonarr, so a double-click or two concurrent admins can't both add the same content; releases the claim back to pending on any failure so a fixable error can be retried. - Music enrichment's update_many now re-checks album_art_url is still empty, so it no longer clobbers art an admin manually set via PATCH. - Library Cleanup's dangling-refs scan now also checks episode_progress for rows referencing a deleted episode (previously only watchlist/ progress against movie_id were checked). - Settings now surfaces a failed bulk-TMDB-enrich run as an error instead of a false "Enrichment done" success summary. - Music player's isPlaying now syncs from the audio element's native play/pause events instead of only being set inside togglePlay/track- change, so it can't drift from actual playback state (buffering stalls, OS media-key pauses, etc). --- backend/library_cleanup.py | 13 ++++- backend/server.py | 97 ++++++++++++++++++-------------- frontend/src/lib/musicPlayer.jsx | 15 +++-- frontend/src/pages/Settings.jsx | 13 ++++- 4 files changed, 89 insertions(+), 49 deletions(-) diff --git a/backend/library_cleanup.py b/backend/library_cleanup.py index 37d57f5..13fb122 100644 --- a/backend/library_cleanup.py +++ b/backend/library_cleanup.py @@ -102,6 +102,12 @@ async def scan(db, videos_dir: Path, hls_dir: Path) -> Dict[str, List[Dict]]: if r.get("movie_id") and r["movie_id"] not in movie_ids: findings["dangling_refs"].append({"id": r["id"] if "id" in r else f"{r['profile_id']}:{r['movie_id']}", "content_type": coll_name, "label": r["movie_id"], "detail": f"references deleted movie {r['movie_id']}", "profile_id": r.get("profile_id"), "movie_id": r.get("movie_id")}) + # --- Dangling episode_progress rows (reference a deleted episode) --- + ep_rows = await db.episode_progress.find({}, {"_id": 0}).to_list(20000) + for r in ep_rows: + if r.get("episode_id") and r["episode_id"] not in episode_ids: + findings["dangling_refs"].append({"id": f"{r['profile_id']}:{r['episode_id']}", "content_type": "episode_progress", "label": r["episode_id"], "detail": f"references deleted episode {r['episode_id']}", "profile_id": r.get("profile_id"), "episode_id": r.get("episode_id")}) + return findings @@ -155,8 +161,11 @@ async def fix_one(db, videos_dir: Path, hls_dir: Path, category: str, item: Dict await db.subtitles.delete_one({"id": item_id}) elif category == "dangling_refs": - coll = {"watchlist": db.watchlist, "progress": db.progress}[content_type] - await coll.delete_one({"profile_id": item.get("profile_id"), "movie_id": item.get("movie_id")}) + if content_type == "episode_progress": + await db.episode_progress.delete_one({"profile_id": item.get("profile_id"), "episode_id": item.get("episode_id")}) + else: + coll = {"watchlist": db.watchlist, "progress": db.progress}[content_type] + await coll.delete_one({"profile_id": item.get("profile_id"), "movie_id": item.get("movie_id")}) else: raise ValueError(f"Unknown cleanup category: {category}") diff --git a/backend/server.py b/backend/server.py index cedbef6..a198694 100644 --- a/backend/server.py +++ b/backend/server.py @@ -692,8 +692,9 @@ async def cancel_job(job_id: str, user: dict = Depends(require_admin)): raise HTTPException(status_code=400, detail="Cannot cancel a running job") await db.transcode_queue.update_one({"id": job_id}, {"$set": {"status": "cancelled", "finished_at": datetime.now(timezone.utc).isoformat()}}) if job["status"] == "pending": - # Reset movie hls_status AND clear stale hls_path so the row no longer shows as transcoded - await db.movies.update_one({"id": job["movie_id"]}, {"$set": {"hls_status": None, "hls_path": None}}) + # Reset hls_status AND clear stale hls_path so the row no longer shows as transcoded + coll = db.movies if job.get("content_type", "movie") == "movie" else db.episodes + await coll.update_one({"id": job["movie_id"]}, {"$set": {"hls_status": None, "hls_path": None}}) return {"ok": True} @@ -703,7 +704,7 @@ async def retry_job(job_id: str, user: dict = Depends(require_admin)): if not job: raise HTTPException(status_code=404, detail="Job not found") if job["status"] not in ("failed", "cancelled"): raise HTTPException(status_code=400, detail="Only failed/cancelled jobs can be retried") - new_job = await _enqueue_transcode(job["movie_id"], job["quality"], triggered_by=job.get("triggered_by", "manual")) + new_job = await _enqueue_transcode(job["movie_id"], job["quality"], triggered_by=job.get("triggered_by", "manual"), content_type=job.get("content_type", "movie")) return {"ok": True, "job_id": new_job["id"]} @@ -935,44 +936,58 @@ async def approve_request(request_id: str, payload: RequestApprove, user: dict = """Looks up the request's title in Radarr/Sonarr, adds the best match with an immediate search enabled, and marks the request approved. Content actually lands via the normal Radarr/Sonarr download pipeline — this only kicks that off.""" - req = await db.requests.find_one({"id": request_id}, {"_id": 0}) - if not req: raise HTTPException(status_code=404, detail="Request not found") if payload.content_type not in ("movie", "tv"): raise HTTPException(status_code=400, detail="content_type must be movie or tv") + # Atomically claim the request so two concurrent approve calls (double-click, two admins) + # can't both pass a check-then-act race and each add the content to Radarr/Sonarr. + req = await db.requests.find_one_and_update( + {"id": request_id, "status": "pending"}, {"$set": {"status": "approving"}}, + projection={"_id": 0}, return_document=False, + ) + if not req: + existing = await db.requests.find_one({"id": request_id}, {"_id": 0, "status": 1}) + if not existing: raise HTTPException(status_code=404, detail="Request not found") + raise HTTPException(status_code=409, detail=f"Request is already {existing['status']}") s = await get_settings() - if payload.content_type == "movie": - if not s.get("radarr_url") or not s.get("radarr_api_key"): - raise HTTPException(status_code=400, detail="Radarr not configured") - try: - candidates = await asyncio.to_thread(radarr_client.search_movie, s["radarr_url"], s["radarr_api_key"], req["title"]) - except Exception as e: - raise HTTPException(status_code=502, detail=f"Radarr lookup error: {e}") - match = next((c for c in candidates if req.get("year") and c.get("year") == req["year"]), candidates[0] if candidates else None) - if not match: - raise HTTPException(status_code=404, detail="No match found in Radarr") - try: - added = await asyncio.to_thread(radarr_client.add_movie, s["radarr_url"], s["radarr_api_key"], match["tmdb_id"], match["title"], match.get("year")) - except Exception as e: - raise HTTPException(status_code=502, detail=f"Radarr add error: {e}") - updates = {"status": "approved", "content_type": "movie", "radarr_id": added["radarr_id"], - "matched_title": added.get("title"), "matched_year": added.get("year")} - else: - if not s.get("sonarr_url") or not s.get("sonarr_api_key"): - raise HTTPException(status_code=400, detail="Sonarr not configured") - try: - candidates = await asyncio.to_thread(sonarr_client.search_series, s["sonarr_url"], s["sonarr_api_key"], req["title"]) - except Exception as e: - raise HTTPException(status_code=502, detail=f"Sonarr lookup error: {e}") - match = next((c for c in candidates if req.get("year") and c.get("year") == req["year"]), candidates[0] if candidates else None) - if not match: - raise HTTPException(status_code=404, detail="No match found in Sonarr") - try: - added = await asyncio.to_thread(sonarr_client.add_series, s["sonarr_url"], s["sonarr_api_key"], match["tvdb_id"], match["title"], match.get("year")) - except Exception as e: - raise HTTPException(status_code=502, detail=f"Sonarr add error: {e}") - updates = {"status": "approved", "content_type": "tv", "sonarr_id": added["sonarr_id"], - "matched_title": added.get("title"), "matched_year": added.get("year")} + try: + if payload.content_type == "movie": + if not s.get("radarr_url") or not s.get("radarr_api_key"): + raise HTTPException(status_code=400, detail="Radarr not configured") + try: + candidates = await asyncio.to_thread(radarr_client.search_movie, s["radarr_url"], s["radarr_api_key"], req["title"]) + except Exception as e: + raise HTTPException(status_code=502, detail=f"Radarr lookup error: {e}") + match = next((c for c in candidates if req.get("year") and c.get("year") == req["year"]), candidates[0] if candidates else None) + if not match: + raise HTTPException(status_code=404, detail="No match found in Radarr") + try: + added = await asyncio.to_thread(radarr_client.add_movie, s["radarr_url"], s["radarr_api_key"], match["tmdb_id"], match["title"], match.get("year")) + except Exception as e: + raise HTTPException(status_code=502, detail=f"Radarr add error: {e}") + updates = {"status": "approved", "content_type": "movie", "radarr_id": added["radarr_id"], + "matched_title": added.get("title"), "matched_year": added.get("year")} + else: + if not s.get("sonarr_url") or not s.get("sonarr_api_key"): + raise HTTPException(status_code=400, detail="Sonarr not configured") + try: + candidates = await asyncio.to_thread(sonarr_client.search_series, s["sonarr_url"], s["sonarr_api_key"], req["title"]) + except Exception as e: + raise HTTPException(status_code=502, detail=f"Sonarr lookup error: {e}") + match = next((c for c in candidates if req.get("year") and c.get("year") == req["year"]), candidates[0] if candidates else None) + if not match: + raise HTTPException(status_code=404, detail="No match found in Sonarr") + try: + added = await asyncio.to_thread(sonarr_client.add_series, s["sonarr_url"], s["sonarr_api_key"], match["tvdb_id"], match["title"], match.get("year")) + except Exception as e: + raise HTTPException(status_code=502, detail=f"Sonarr add error: {e}") + updates = {"status": "approved", "content_type": "tv", "sonarr_id": added["sonarr_id"], + "matched_title": added.get("title"), "matched_year": added.get("year")} + except HTTPException: + # Release the claim so a fixable failure (e.g. Radarr temporarily down) can be retried, + # instead of leaving the request stuck in "approving" forever. + await db.requests.update_one({"id": request_id}, {"$set": {"status": "pending"}}) + raise res = await db.requests.find_one_and_update( {"id": request_id}, {"$set": updates}, projection={"_id": 0}, return_document=True, @@ -1212,7 +1227,7 @@ async def bulk_transcode_eps(payload: dict, user: dict = Depends(require_admin)) for eid in ids: ep = await db.episodes.find_one({"id": eid}, {"_id": 0}) if not ep or ep.get("hls_status") in ("running", "pending"): continue - if ep.get("storage_type") not in ("local", "sonarr") or not ep.get("storage_path"): continue + if ep.get("storage_type") not in ("local", "sonarr", "scan") or not ep.get("storage_path"): continue await _enqueue_transcode(eid, quality, triggered_by="manual", content_type="episode") queued += 1 return {"ok": True, "queued": queued} @@ -1244,8 +1259,8 @@ async def transcode_episode(episode_id: str, payload: Optional[dict] = None, use if quality not in ("quick", "abr"): raise HTTPException(status_code=400, detail="Invalid quality") ep = await db.episodes.find_one({"id": episode_id}, {"_id": 0}) if not ep: raise HTTPException(status_code=404, detail="Episode not found") - if ep.get("storage_type") not in ("local", "sonarr") or not ep.get("storage_path"): - raise HTTPException(status_code=400, detail="Only local/sonarr episodes can be transcoded") + if ep.get("storage_type") not in ("local", "sonarr", "scan") or not ep.get("storage_path"): + raise HTTPException(status_code=400, detail="Only local/sonarr/scanned episodes can be transcoded") if ep.get("hls_status") in ("running", "pending"): raise HTTPException(status_code=409, detail="Already transcoding or queued") job = await _enqueue_transcode(episode_id, quality, triggered_by="manual", content_type="episode") @@ -1591,7 +1606,7 @@ async def _run_music_enrich(): art_url = await asyncio.to_thread(musicbrainz_client.cover_art_url, release["mbid"]) if art_url: res = await db.tracks.update_many( - {"artist": artist, "album": album}, + {"artist": artist, "album": album, "album_art_url": {"$in": [None, ""]}}, {"$set": {"album_art_url": art_url}}, ) _music_enrich_status["updated"] += res.modified_count diff --git a/frontend/src/lib/musicPlayer.jsx b/frontend/src/lib/musicPlayer.jsx index 36a605f..03f7053 100644 --- a/frontend/src/lib/musicPlayer.jsx +++ b/frontend/src/lib/musicPlayer.jsx @@ -41,29 +41,36 @@ export const MusicPlayerProvider = ({ children }) => { const audio = audioRef.current; if (!current) { audio.pause(); return; } audio.src = getTrackStreamUrl(current); - audio.play().then(() => setIsPlaying(true)).catch(() => setIsPlaying(false)); + audio.play().catch(() => {}); // isPlaying reflects reality via the native play/pause listeners below }, [current]); + // isPlaying always mirrors the audio element's own state, so it stays correct through anything + // that changes playback outside togglePlay — a stall, an OS media-key pause, autoplay being blocked. useEffect(() => { const audio = audioRef.current; const onTime = () => setProgress({ position: audio.currentTime, duration: audio.duration || 0 }); const onEnded = () => { if (index >= 0 && index < queue.length - 1) setIndex(index + 1); - else setIsPlaying(false); }; + const onPlay = () => setIsPlaying(true); + const onPause = () => setIsPlaying(false); audio.addEventListener("timeupdate", onTime); audio.addEventListener("ended", onEnded); + audio.addEventListener("play", onPlay); + audio.addEventListener("pause", onPause); return () => { audio.removeEventListener("timeupdate", onTime); audio.removeEventListener("ended", onEnded); + audio.removeEventListener("play", onPlay); + audio.removeEventListener("pause", onPause); }; }, [index, queue]); const togglePlay = useCallback(() => { const audio = audioRef.current; if (!current) return; - if (audio.paused) audio.play().then(() => setIsPlaying(true)); - else { audio.pause(); setIsPlaying(false); } + if (audio.paused) audio.play().catch(() => {}); + else audio.pause(); }, [current]); const next = useCallback(() => { diff --git a/frontend/src/pages/Settings.jsx b/frontend/src/pages/Settings.jsx index 9014da5..fc48796 100644 --- a/frontend/src/pages/Settings.jsx +++ b/frontend/src/pages/Settings.jsx @@ -140,7 +140,11 @@ export default function Settings() { setEnrich(data); if (!data.running) { clearInterval(t); - toast.success(`Enrichment done — ${data.enriched} enriched, ${data.skipped} already complete, ${data.failed} not found`); + if (data.error) { + toast.error(`Enrichment failed: ${data.error}`); + } else { + toast.success(`Enrichment done — ${data.enriched} enriched, ${data.skipped} already complete, ${data.failed} not found`); + } } }, 2000); return () => clearInterval(t); @@ -210,7 +214,12 @@ export default function Settings() { - {enrich && !enrich.running && enrich.total > 0 && ( + {enrich && !enrich.running && enrich.error && ( + + Last run failed: {enrich.error} + + )} + {enrich && !enrich.running && !enrich.error && enrich.total > 0 && ( Last run: {enrich.enriched} enriched · {enrich.skipped} already complete · {enrich.failed} not found on TMDB