From 2a7f49c981ff7c6137931fbb5aed27476516ad41 Mon Sep 17 00:00:00 2001 From: npeter83 Date: Thu, 23 Jul 2026 23:25:06 +0200 Subject: [PATCH] =?UTF-8?q?fix(r5):=20address=20the=20/code-review=20high?= =?UTF-8?q?=20round=20=E2=80=94=2010=20findings?= MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit - HLS sweep only deletes directories matching the session-name shape; the root is admin-configurable and may be a shared path - playlist unwrap picks the first entry that actually downloaded, not entries[0] - concat quoting keeps backslashes literal (ffmpeg treats them so inside '') - ArrowUp/Down defer while the focused element can still scroll that way - the staging fallback skips yt-dlp working files (.part-Frag/.ytdl/.temp/.fNNN) - the boot HLS sweep runs off the event loop - volume persistence is debounced (was a setItem per wheel notch) and a spin no longer loses notches to the iframe's lagging getVolume - _Breaker.run owns the whole open/call/record protocol; four call sites shrink - should_prune extracted and unit-tested, plus RSS error-vs-empty tests - the worker exits non-zero when the schema never lands --- backend/app/downloads/edit.py | 11 +-- backend/app/main.py | 7 +- backend/app/plex/stream.py | 20 ++++-- backend/app/sync/playlists.py | 85 ++++++++++++---------- backend/app/sync/subscriptions.py | 21 ++++-- backend/app/worker.py | 39 +++++++--- backend/tests/test_download_output.py | 38 +++++++++- backend/tests/test_sync_guards.py | 96 ++++++++++++++++++++++++- frontend/src/components/PlayerModal.tsx | 69 +++++++++++++++--- 9 files changed, 307 insertions(+), 79 deletions(-) diff --git a/backend/app/downloads/edit.py b/backend/app/downloads/edit.py index 395721d..97f102b 100644 --- a/backend/app/downloads/edit.py +++ b/backend/app/downloads/edit.py @@ -184,11 +184,12 @@ def build_edit_cmd(src: Path, dest: Path, spec: dict, out_ext: str) -> list[str] def _concat_quote(path: Path) -> str: """Escape a path for a single-quoted `file '…'` line of an ffmpeg concat list. - The demuxer's own rule: inside single quotes a literal `'` is written `'\\''` (close, escaped - quote, reopen) and a `\\` must be doubled. On-disk names have been ASCII-slugged since 0.51.0, - but that change is FORWARD-ONLY — a pre-0.51.0 file still on disk can be named `Don't_….mp4`, - and an unescaped apostrophe there makes the whole list unparseable.""" - return path.as_posix().replace("\\", "\\\\").replace("'", "'\\''") + ffmpeg's quoting rule: inside single quotes EVERY character is literal except `'` itself, which + is written `'\\''` (close the quote, emit an escaped quote, reopen). Backslashes are literal + there — doubling them would corrupt a path that contains one. On-disk names have been + ASCII-slugged since 0.51.0, but that change is FORWARD-ONLY: a pre-0.51.0 file still on disk can + be named `Don't_….mp4`, and an unescaped apostrophe makes the whole list unparseable.""" + return path.as_posix().replace("'", "'\\''") def build_concat_plan(src: Path, dest: Path, spec: dict, out_ext: str, staging: Path): diff --git a/backend/app/main.py b/backend/app/main.py index 45643c4..e419b3a 100644 --- a/backend/app/main.py +++ b/backend/app/main.py @@ -1,3 +1,4 @@ +import asyncio import logging import sys from contextlib import asynccontextmanager @@ -69,8 +70,10 @@ async def lifespan(app: FastAPI): url, ) # No HLS session survives a restart (they live in this process), so anything still on disk is - # a crashed run's segments — sweep them or they accumulate forever. - swept = plex_stream.sweep_orphans() + # a crashed run's segments — sweep them or they accumulate forever. Off the event loop: the + # first sweep after a long uptime can be gigabytes of `.ts` files, and blocking here delays + # serving (a 502 + a failed post-deploy health probe). + swept = await asyncio.to_thread(plex_stream.sweep_orphans) if swept: log.info("swept %d orphaned HLS segment directories", swept) start_scheduler() diff --git a/backend/app/plex/stream.py b/backend/app/plex/stream.py index 4401db1..198704f 100644 --- a/backend/app/plex/stream.py +++ b/backend/app/plex/stream.py @@ -15,6 +15,7 @@ segments. The remux is CPU-light (video copy + a tiny audio transcode), so it ru CPU-only prod host; only P3's full transcode is CPU-heavy. """ import logging +import re import shutil import subprocess import threading @@ -33,6 +34,11 @@ log = logging.getLogger("siftlode.plex") _HLS_ROOT = Path(settings.plex_hls_dir) if settings.plex_hls_dir else Path(settings.download_root) / ".plex-hls" _SEG_SECONDS = 6 _SESSION_IDLE_S = 600 # reap a session with no access for this long +# A session directory is named `{rating_key}_{int(start_s)}_{tag}_{aoff:+.2f}` (see start_session). +# `sweep_orphans` deletes ONLY names matching this shape: `_HLS_ROOT` is admin-configurable +# (PLEX_HLS_DIR), so it may legitimately point at a shared scratch path we do not own — a blanket +# "delete every directory under the root" would take the neighbours with it. +_SESSION_DIR_RE = re.compile(r"^.+_\d+_[^_]+_[+-]\d+\.\d{2}$") _lock = threading.Lock() _sessions: dict[str, "HlsSession"] = {} @@ -255,12 +261,16 @@ def wait_for(path: Path, timeout: float = 20.0) -> bool: def sweep_orphans() -> int: - """Delete segment directories no live session owns. + """Delete OUR segment directories that no live session owns. - Sessions live only in THIS process, so at startup every directory under `_HLS_ROOT` is a - leftover from a crashed or killed run — and nothing else ever reaps them (the download GC + Sessions live only in THIS process, so at startup every session directory under `_HLS_ROOT` is + a leftover from a crashed or killed run — and nothing else ever reaps them (the download GC deliberately skips `.plex-hls` as a system tree), so a few restarts mid-playback leave - gigabytes of `.ts` segments behind forever.""" + gigabytes of `.ts` segments behind forever. + + Only entries whose name matches `_SESSION_DIR_RE` are touched: the root is admin-configurable, + so anything else under it belongs to someone else and is left alone. Blocking (rmtree) — call + it off the event loop.""" with _lock: live = {s.dir for s in _sessions.values()} dropped = 0 @@ -269,7 +279,7 @@ def sweep_orphans() -> int: except OSError: # the root doesn't exist yet — nothing to sweep return 0 for p in entries: - if p.is_dir() and p not in live: + if p.is_dir() and p not in live and _SESSION_DIR_RE.match(p.name): shutil.rmtree(p, ignore_errors=True) dropped += 1 return dropped diff --git a/backend/app/sync/playlists.py b/backend/app/sync/playlists.py index 82926c7..27e4049 100644 --- a/backend/app/sync/playlists.py +++ b/backend/app/sync/playlists.py @@ -7,6 +7,7 @@ never synced. Videos in a playlist that aren't in our shared catalog yet are fet ingested (with a stub channel) so the mirror is faithful.""" import hashlib import logging +from collections.abc import Callable from datetime import datetime, timezone from sqlalchemy import delete, select @@ -271,8 +272,16 @@ def plan_push(db: Session, user: User, pl: Playlist, *, live=None) -> dict: _MAX_CONSECUTIVE_FAILURES = 5 +#: Returned by `_Breaker.run` when the write did not happen (refused or failed). +FAILED = object() + + class _Breaker: - """Consecutive-failure circuit breaker for a single push run. Any success re-arms it.""" + """Consecutive-failure circuit breaker for a single push run. Any success re-arms it. + + Drive it with `run()` — it owns the whole open-check / call / record-outcome sequence, so a + write site can't accidentally implement two thirds of the protocol (forgetting to record a + failure would silently disable the breaker for that operation).""" def __init__(self, limit: int = _MAX_CONSECUTIVE_FAILURES) -> None: self.limit = limit @@ -296,6 +305,22 @@ class _Breaker: self.limit, ) + def run(self, op: Callable[[], object], on_fail: Callable[[], None]) -> object: + """Attempt one YouTube write. Returns the op's result, or `FAILED` when the breaker is + already tripped (op not called) or the op raised YouTubeError. `on_fail` records the item + in the caller's failure list either way.""" + if self.tripped: + on_fail() + return FAILED + try: + out = op() + except YouTubeError: + on_fail() + self.hit(False) + return FAILED + self.hit(True) + return out + def push_playlist(db: Session, user: User, pl: Playlist, *, live=None) -> dict: """Push a local playlist to YouTube: create it if needed, then reconcile membership @@ -313,16 +338,11 @@ def push_playlist(db: Session, user: User, pl: Playlist, *, live=None) -> dict: created = 1 # Brand-new playlist: just append every video in order. for vid in desired: - if not breaker.open(): - failures.append(vid) - continue - try: - yt.add_playlist_item(pl.yt_playlist_id, vid) + if breaker.run( + lambda v=vid: yt.add_playlist_item(pl.yt_playlist_id, v), + lambda v=vid: failures.append(v), + ) is not FAILED: inserted += 1 - breaker.hit(True) - except YouTubeError: - failures.append(vid) - breaker.hit(False) if not failures: pl.synced_fingerprint = fingerprint(pl.name, desired) pl.dirty = False @@ -354,30 +374,22 @@ def push_playlist(db: Session, user: User, pl: Playlist, *, live=None) -> dict: # Delete extras (local is authoritative). for it in current: if it["video_id"] not in desired_set: - if not breaker.open(): - failures.append(it["video_id"]) - continue - try: - yt.delete_playlist_item(it["item_id"]) + if breaker.run( + lambda i=it: yt.delete_playlist_item(i["item_id"]), + lambda i=it: failures.append(i["video_id"]), + ) is not FAILED: deleted += 1 - breaker.hit(True) - except YouTubeError: - failures.append(it["video_id"]) - breaker.hit(False) # Insert missing (append; the reorder pass below fixes positions). cur_set = set(cur_vids) for vid in desired: if vid not in cur_set: - if not breaker.open(): - failures.append(vid) - continue - try: - item_id_by_vid[vid] = yt.add_playlist_item(pl.yt_playlist_id, vid) + item_id = breaker.run( + lambda v=vid: yt.add_playlist_item(pl.yt_playlist_id, v), + lambda v=vid: failures.append(v), + ) + if item_id is not FAILED: + item_id_by_vid[vid] = item_id inserted += 1 - breaker.hit(True) - except YouTubeError: - failures.append(vid) - breaker.hit(False) # Reorder to match `desired` via insertion-sort moves over a local model. model = [v for v in cur_vids if v in desired_set] + [ v for v in desired if v not in cur_set and v in item_id_by_vid @@ -387,17 +399,14 @@ def push_playlist(db: Session, user: User, pl: Playlist, *, live=None) -> dict: continue # failed insert; skip if i < len(model) and model[i] == want: continue - if not breaker.open(): - failures.append(want) - continue - try: - yt.move_playlist_item(item_id_by_vid[want], pl.yt_playlist_id, want, i) - reordered += 1 - breaker.hit(True) - except YouTubeError: - failures.append(want) - breaker.hit(False) + if breaker.run( + lambda w=want, pos=i: yt.move_playlist_item( + item_id_by_vid[w], pl.yt_playlist_id, w, pos + ), + lambda w=want: failures.append(w), + ) is FAILED: continue + reordered += 1 j = model.index(want) model.pop(j) model.insert(i, want) diff --git a/backend/app/sync/subscriptions.py b/backend/app/sync/subscriptions.py index 1d95ec7..019445a 100644 --- a/backend/app/sync/subscriptions.py +++ b/backend/app/sync/subscriptions.py @@ -73,6 +73,18 @@ def apply_channel_details(db: Session, items: list[dict]) -> None: channel.details_synced_at = now +def should_prune(fetched_count: int, existing_count: int) -> bool: + """Whether an import may delete the local subscriptions missing from this fetch. + + NEVER on an empty fetch that contradicts local state: the API can answer 200 with no items (a + transient blip, a scope/token edge), and pruning against that wipes every subscription the user + has — taking each row's `deep_requested` flag with it — in a single run. Someone who genuinely + unsubscribed from everything simply keeps their rows until a sync that returns something. + + Pure so it can be tested without a database; `import_subscriptions` is the only caller.""" + return fetched_count > 0 or existing_count == 0 + + def import_subscriptions(db: Session, user: User) -> dict: """Pull all subscriptions for `user`, upsert channels + subscription links, prune channels the user has unsubscribed from on YouTube, and fetch channel details for @@ -108,18 +120,15 @@ def import_subscriptions(db: Session, user: User) -> dict: sub_row.yt_subscription_id = sub.get("yt_subscription_id") db.flush() - # Prune subscriptions removed on YouTube — but NEVER on an empty fetch. The API can answer - # 200 with no items (a transient blip, a scope/token edge), and pruning against that wipes - # every subscription the user has, taking each row's `deep_requested` flag with it, in one - # run. Someone who genuinely unsubscribed from everything simply keeps their rows until a - # sync that returns something. + # Prune subscriptions removed on YouTube — but only when this fetch is trustworthy + # (see `should_prune`). removed = 0 existing = ( db.execute(select(Subscription).where(Subscription.user_id == user.id)) .scalars() .all() ) - prune_skipped = bool(existing) and not fetched + prune_skipped = not should_prune(len(fetched), len(existing)) if prune_skipped: log.warning( "Subscription import (user %s): YouTube returned 0 subscriptions while %d exist " diff --git a/backend/app/worker.py b/backend/app/worker.py index 00c1787..e319a55 100644 --- a/backend/app/worker.py +++ b/backend/app/worker.py @@ -16,6 +16,7 @@ Design: * Crash-safe: on startup, orphaned `running` jobs are requeued and `downloading` assets reset. """ import logging +import re import shutil import signal import threading @@ -351,6 +352,17 @@ def _staging_dir(asset_id: int) -> Path: _THUMB_SUFFIXES = (".jpg", ".jpeg", ".png", ".webp") +# yt-dlp's working files in the staging dir. None of them is ever the finished media, but several +# CAN be the biggest file there (a video-only per-format stream, an abandoned fragment run), so the +# staging fallback must exclude them by name — picking one would store e.g. a silent video-only +# stream as a ready asset. +_YTDLP_TEMP_RE = re.compile( + r"\.part(-Frag\d+)?$" # in-flight download / fragment + r"|\.ytdl$" # resume state + r"|\.temp\.[^.]+$" # merge/postprocess temp + r"|\.f\d+\.[^.]+$", # per-format stream kept before the merge + re.IGNORECASE, +) def _find_thumbnail(staging: Path, media: Path) -> Path | None: @@ -367,7 +379,8 @@ def _produced_file(info: dict, staging: Path) -> Path: extractor may not fill it in at all — and `Path("")` is `Path(".")`, which *passes* `.exists()`, so a missing value used to sail past the fallback and blow up two steps later (`with_name` on an empty name: the BBC-news failure). Anything that isn't a real file counts as absent; then fall - back to the largest non-thumbnail file left in staging.""" + back to the largest file left in staging that is neither a thumbnail nor one of yt-dlp's own + working files (see `_YTDLP_TEMP_RE`).""" req = (info.get("requested_downloads") or [{}])[0] fp = req.get("filepath") or req.get("_filename") or info.get("filepath") or info.get("_filename") if fp: @@ -377,7 +390,9 @@ def _produced_file(info: dict, staging: Path) -> Path: cands = [ p for p in staging.iterdir() - if p.is_file() and p.suffix.lower() not in _THUMB_SUFFIXES and not p.name.endswith(".part") + if p.is_file() + and p.suffix.lower() not in _THUMB_SUFFIXES + and not _YTDLP_TEMP_RE.search(p.name) ] if not cands: raise RuntimeError("yt-dlp produced no output file") @@ -388,11 +403,15 @@ def _media_info(info: dict) -> dict: """Unwrap a playlist/multi_video extraction to the entry that carries the download. A single non-YouTube page (a news article's video, a multi-rendition embed) can extract as a - wrapper whose own dict has no `requested_downloads` — the media lives on the first entry.""" + wrapper whose own dict has no `requested_downloads` — the media lives on an entry. Pick the + FIRST entry that actually downloaded something: on a page with a skipped teaser in front of the + real video, entries[0] carries no file, and taking it blindly would label the produced file + with the wrong id/title/duration.""" entries = [e for e in (info.get("entries") or []) if e] - if entries and not info.get("requested_downloads"): - return {**info, **entries[0]} - return info + if not entries or info.get("requested_downloads"): + return info + chosen = next((e for e in entries if e.get("requested_downloads")), entries[0]) + return {**info, **chosen} # Errors worth retrying with a different player-client set (YouTube per-client flakiness). @@ -729,9 +748,13 @@ def main() -> None: return # The API may still be applying migrations when we boot. If they never arrive, exit instead of - # pressing on into a guaranteed crash — the container's restart policy retries the whole wait. + # pressing on into a guaranteed crash — with a FAILURE status, so the container comes back under + # any restart policy (`on-failure` too, not just the `unless-stopped` our composes ship) and the + # exit code doesn't claim success for a worker that never started working. if not _wait_for_schema(): - return + if _stop.is_set(): + return # asked to shut down while waiting — that IS a clean exit + raise SystemExit(1) _recover_orphans() n = max(1, settings.download_worker_concurrency) log.info("Download worker started (concurrency=%d, root=%s).", n, settings.download_root) diff --git a/backend/tests/test_download_output.py b/backend/tests/test_download_output.py index d48bcad..3f9e51c 100644 --- a/backend/tests/test_download_output.py +++ b/backend/tests/test_download_output.py @@ -38,6 +38,22 @@ class TestProducedFile: (tmp_path / "huge.mp4.part").write_bytes(b"x" * 999) assert _produced_file({}, staging) == tmp_path / "big.mp4" + @pytest.mark.parametrize( + "leftover", + ["vid.f137.mp4", "vid.mp4.part-Frag12", "vid.ytdl", "vid.temp.mp4", "vid.mp4.part"], + ) + def test_ytdlp_working_files_never_win(self, tmp_path, leftover): + # Each of these can legitimately be the BIGGEST file in staging (a video-only per-format + # stream especially) — storing one would serve a silent or truncated "download". + (tmp_path / leftover).write_bytes(b"x" * 5000) + (tmp_path / "vid.mp4").write_bytes(b"x" * 10) + assert _produced_file({}, tmp_path) == tmp_path / "vid.mp4" + + def test_only_leftovers_is_treated_as_no_output(self, tmp_path): + (tmp_path / "vid.f251.webm").write_bytes(b"x" * 5000) + with pytest.raises(RuntimeError): + _produced_file({}, tmp_path) + def test_no_output_at_all_raises(self, tmp_path): staging = _staging(tmp_path, "only.jpg") with pytest.raises(RuntimeError): @@ -53,6 +69,22 @@ class TestMediaInfo: info = {"id": "vid", "requested_downloads": [{"filepath": "/x.mp4"}]} assert _media_info(info) is info + def test_picks_the_entry_that_actually_downloaded(self): + # A page whose first entry was skipped: taking entries[0] would label the produced file + # with the teaser's id/title. + info = { + "id": "page", + "entries": [ + {"id": "teaser", "title": "Teaser"}, + {"id": "main", "title": "Main", "requested_downloads": [{"filepath": "/x.mp4"}]}, + ], + } + assert _media_info(info)["id"] == "main" + + def test_falls_back_to_the_first_entry_when_none_downloaded(self): + info = {"id": "page", "entries": [{"id": "a"}, {"id": "b"}]} + assert _media_info(info)["id"] == "a" + def test_wrapper_that_downloaded_itself_wins_over_its_entries(self): info = {"id": "vid", "requested_downloads": [{"filepath": "/x.mp4"}], "entries": [{"id": "other"}]} assert _media_info(info)["id"] == "vid" @@ -98,8 +130,10 @@ class TestConcatQuote: # A pre-0.51.0 file can still be named with an apostrophe; unescaped it breaks the list. assert _concat_quote(PurePosixPath("/m/Don't_Look.mp4")) == "/m/Don'\\''t_Look.mp4" - def test_backslash_is_doubled(self): - assert _concat_quote(PurePosixPath("/m/a\\b.mp4")) == "/m/a\\\\b.mp4" + def test_backslash_stays_literal(self): + # Inside single quotes ffmpeg treats `\` literally — doubling it would point at a + # different (non-existent) path. + assert _concat_quote(PurePosixPath("/m/a\\b.mp4")) == "/m/a\\b.mp4" def test_ordinary_path_is_unchanged(self): assert _concat_quote(PurePosixPath("/m/clip.mp4")) == "/m/clip.mp4" diff --git a/backend/tests/test_sync_guards.py b/backend/tests/test_sync_guards.py index c887b5d..484f906 100644 --- a/backend/tests/test_sync_guards.py +++ b/backend/tests/test_sync_guards.py @@ -1,5 +1,28 @@ -"""Guards that stop one bad YouTube answer from cascading: the playlist-push circuit breaker.""" -from app.sync.playlists import _Breaker +"""Guards that stop one bad YouTube answer from cascading: the subscription prune guard, the +playlist-push circuit breaker, and RSS telling a failure apart from an empty feed.""" +import httpx +import pytest + +from app.sync.playlists import FAILED, _Breaker +from app.sync.subscriptions import should_prune +from app.youtube import rss +from app.youtube.client import YouTubeError + + +class TestShouldPrune: + def test_a_normal_fetch_prunes(self): + assert should_prune(fetched_count=12, existing_count=15) is True + + def test_an_empty_fetch_against_existing_rows_never_prunes(self): + # The data-loss case: one anomalous empty-but-200 response must not wipe 15 rows. + assert should_prune(fetched_count=0, existing_count=15) is False + + def test_an_empty_fetch_with_nothing_local_is_fine(self): + # A brand-new account: nothing to lose, so the (no-op) prune may run. + assert should_prune(fetched_count=0, existing_count=0) is True + + def test_a_single_remaining_subscription_still_prunes(self): + assert should_prune(fetched_count=1, existing_count=200) is True class TestBreaker: @@ -26,3 +49,72 @@ class TestBreaker: b.hit(False) b.hit(True) assert not b.open() + + +class TestBreakerRun: + def test_returns_the_ops_result_and_leaves_failures_alone(self): + b, failed = _Breaker(), [] + assert b.run(lambda: "item-1", lambda: failed.append("v1")) == "item-1" + assert failed == [] + + def test_a_youtube_error_records_the_failure_and_counts_towards_the_trip(self): + b, failed = _Breaker(limit=2), [] + assert b.run(lambda: (_ for _ in ()).throw(YouTubeError("quota")), lambda: failed.append("v1")) is FAILED + assert failed == ["v1"] and b.open() + b.run(lambda: (_ for _ in ()).throw(YouTubeError("quota")), lambda: failed.append("v2")) + assert not b.open() + + def test_a_tripped_breaker_never_calls_the_op(self): + b, failed, calls = _Breaker(limit=1), [], [] + b.run(lambda: (_ for _ in ()).throw(YouTubeError("quota")), lambda: failed.append("v1")) + assert b.run(lambda: calls.append("called"), lambda: failed.append("v2")) is FAILED + assert calls == [] # the whole point: no more quota spent + assert failed == ["v1", "v2"] # …but the item is still reported + + def test_other_exceptions_are_not_swallowed(self): + # Only YouTubeError is a "write failed" signal; a bug must still surface. + with pytest.raises(ValueError): + _Breaker().run(lambda: (_ for _ in ()).throw(ValueError("bug")), lambda: None) + + +class TestRssErrorVsEmpty: + """A failed poll must RAISE (so the caller leaves `last_rss_at` alone); only a genuinely + empty feed may return [].""" + + def _patch(self, monkeypatch, **kwargs): + def fake_get(url, **_): + if "exc" in kwargs: + raise kwargs["exc"] + return httpx.Response( + kwargs["status"], + content=kwargs.get("body", b""), + request=httpx.Request("GET", url), + ) + + monkeypatch.setattr(rss.httpx, "get", fake_get) + + def test_non_200_raises(self, monkeypatch): + self._patch(monkeypatch, status=404) + with pytest.raises(rss.RssError): + rss.fetch_channel_feed("UC_dead") + + def test_transport_error_raises(self, monkeypatch): + self._patch(monkeypatch, exc=httpx.ConnectError("no route")) + with pytest.raises(rss.RssError): + rss.fetch_channel_feed("UC_offline") + + def test_an_empty_but_valid_feed_returns_empty(self, monkeypatch): + feed = b"""""" + self._patch(monkeypatch, status=200, body=feed) + assert rss.fetch_channel_feed("UC_quiet") == [] + + def test_entries_are_parsed(self, monkeypatch): + feed = b""" + + abc123Hello + 2026-07-23T10:00:00+00:00 + """ + self._patch(monkeypatch, status=200, body=feed) + out = rss.fetch_channel_feed("UC_live") + assert [e["id"] for e in out] == ["abc123"] + assert out[0]["channel_id"] == "UC_live" diff --git a/frontend/src/components/PlayerModal.tsx b/frontend/src/components/PlayerModal.tsx index 96930c1..8e7d268 100644 --- a/frontend/src/components/PlayerModal.tsx +++ b/frontend/src/components/PlayerModal.tsx @@ -55,6 +55,21 @@ type LoopMode = "off" | "one" | "all"; const AUTO_MODES: AutoMode[] = ["off", "next", "prev", "random"]; const LOOP_MODES: LoopMode[] = ["off", "one", "all"]; +/** Can `el` (or an ancestor inside the modal) still scroll vertically in `dir` (-1 up, 1 down)? + * Used to leave ArrowUp/ArrowDown alone while there is content to scroll — the volume shortcut + * must not eat the only keyboard way to read a long description. Stops at ``: the page + * behind the modal is `overflow:hidden` anyway. */ +function canScrollBy(el: HTMLElement | null, dir: -1 | 1): boolean { + for (let node = el; node && node !== document.body; node = node.parentElement) { + const style = getComputedStyle(node); + if (!/(auto|scroll|overlay)/.test(style.overflowY)) continue; + const room = + dir < 0 ? node.scrollTop > 0 : node.scrollTop + node.clientHeight < node.scrollHeight - 1; // -1: sub-pixel heights + if (room) return true; + } + return false; +} + // --- IFrame Player API loader (singleton) --- let apiPromise: Promise | null = null; function loadYouTubeApi(): Promise { @@ -168,9 +183,15 @@ export default function PlayerModal({ const [playerPrefs, patchPlayerPrefs] = useAccountPersistedObject(LS.ytPlayerPrefs, { volume: 100, }); - // The window/wheel listeners below are bound once, so they must read the LIVE prefs. - const prefsRef = useRef(playerPrefs); - prefsRef.current = playerPrefs; + // The live volume: updated on every notch, while the localStorage write behind it is debounced + // (a wheel spin fires ~16 events a second, and each patch is a synchronous setItem on the frame + // the player is decoding on). Everything that READS the level reads this ref, so it never sees + // the stale persisted value mid-spin. The window/wheel listeners are bound once, so a ref — not + // state — is what they can safely close over. + const volumeRef = useRef(playerPrefs.volume); + const volSaveTimerRef = useRef(undefined); + // When the last volume gesture happened — see nudgeVolume (a spin must not re-read the player). + const lastNudgeAtRef = useRef(0); const focusModal = () => cardRef.current?.focus(); const togglePlay = () => { @@ -193,6 +214,11 @@ export default function PlayerModal({ }; /** Push a level (0–100) to the player and remember it; 0 also mutes (a player left at zero * volume but unmuted shows YouTube's speaker icon as ON, which reads as broken). */ + const persistVolume = () => { + window.clearTimeout(volSaveTimerRef.current); + volSaveTimerRef.current = undefined; + patchPlayerPrefs({ volume: volumeRef.current }); + }; const applyVolume = (level: number, flash = true) => { const next = Math.max(0, Math.min(100, Math.round(level))); const p = playerRef.current; @@ -201,15 +227,29 @@ export default function PlayerModal({ p.setVolume(next); if (next === 0) p.mute?.(); } - patchPlayerPrefs({ volume: next }); + volumeRef.current = next; + // Debounced: one write when the spin settles, not one per notch. The unmount cleanup flushes + // a pending write, so the last change is never lost. + window.clearTimeout(volSaveTimerRef.current); + volSaveTimerRef.current = window.setTimeout(persistVolume, 400); if (flash) flashVolume(next); }; const nudgeVolume = (delta: number) => { const p = playerRef.current; if (!p || typeof p.getVolume !== "function") return; - // Read the live player (the user may have used YouTube's own slider), falling back to the - // stored level when the player can't answer yet. - const cur = p.isMuted?.() ? 0 : (p.getVolume?.() ?? prefsRef.current.volume); + // Whose level do we step from? Our own ref, EXCEPT when the gesture starts fresh — then read + // the player, because the user may have moved YouTube's own slider in the meantime. + // `getVolume()` is answered across the iframe boundary and lags our `setVolume` by a beat, so + // re-reading it mid-spin resurrects a stale level and silently drops notches (measured: five + // wheel notches moved 100 → 90 instead of 75). + const now = Date.now(); + const continuing = now - lastNudgeAtRef.current < 1000; + lastNudgeAtRef.current = now; + const cur = continuing + ? volumeRef.current + : p.isMuted?.() + ? 0 + : (p.getVolume?.() ?? volumeRef.current); applyVolume(cur + delta); }; @@ -425,10 +465,14 @@ export default function PlayerModal({ togglePlay(); } else if (e.key === "ArrowUp" || e.key === "ArrowDown") { // Volume by keyboard (YouTube/Plex-style ±5). The only volume control that survives - // fullscreen, where the wheel goes to YouTube's own iframe. - if (typing) return; + // fullscreen, where the wheel goes to YouTube's own iframe. But the modal card scrolls + // (a long description) and Up/Down is how you scroll it from the keyboard — so defer + // whenever the focused element can still scroll that way, exactly like Space defers to a + // focused button. + const up = e.key === "ArrowUp"; + if (typing || canScrollBy(el, up ? -1 : 1)) return; e.preventDefault(); - nudgeVolume(e.key === "ArrowUp" ? 5 : -5); + nudgeVolume(up ? 5 : -5); } else if (e.key === "ArrowLeft" || e.key === "ArrowRight") { // Plain arrows seek within the video (YouTube-style ±5s); Shift+arrow steps to the // previous/next video in the queue (the feed's order or a playlist). @@ -453,6 +497,9 @@ export default function PlayerModal({ window.clearTimeout(closeTimer.current); window.clearTimeout(openTimer.current); window.clearTimeout(volTimerRef.current); + // Flush a debounced volume write — closing the player right after a wheel spin (the normal + // way to leave) must not drop the level the user just set. + if (volSaveTimerRef.current != null) persistVolume(); }; }, [onClose]); @@ -582,7 +629,7 @@ export default function PlayerModal({ // restore the remembered volume — a fresh player always starts at 100. onReady: () => { focusModal(); - applyVolume(prefsRef.current.volume, false); + applyVolume(volumeRef.current, false); }, onStateChange: (e: any) => { // 1 === playing → sync the navigated-to video's title/author for display.