Code-review follow-ups on the S2 factory:
- add queryKeys.test.ts pinning every branch (bare vs param, null-segment
preservation, single-id, variadic composite spreads) — the contract the
epic exists to protect now has a test (suite 68 -> 73)
- unify feed/feedCount/facets on the `!== undefined` guard the rest of the
factory already uses (behaviour-identical for the always-truthy FeedFilters,
removes the collapse-on-null footgun)
Two findings skipped with rationale: variadic composite-key typing (heterogeneous
object/array segments make precise tuples high-effort; root centralized + shapes
now tested) and a pre-existing thread(null) no-op (unreachable in practice).
Extend lib/queryKeys.ts to the remaining ~34 key roots and migrate every
remaining literal (queryKey: [...] AND the positional useLiveQuery keys) in
36 files. All ~200 React Query keys across the app now route through the
factory; zero raw key literals remain outside queryKeys.ts.
The heterogeneous composite plex keys (plex-library/facets/collections/
playlists carry a filters object, a ratingKeys array, or a union/group
discriminator) take a variadic pass-through — the factory centralizes the
ROOT string; single-id keys stay precisely typed. Nullable keys (thread,
playlist, ytSearch) accept null so a null segment is preserved.
Pure substitution — byte-identical key arrays, so TanStack prefix matching
is unchanged. Gate green: typecheck (app+node+e2e), eslint (0 err),
prettier, 68 vitest. Runtime-verified on a fresh dev server: Feed + Plex
modules load, feed/facets/tags/my-status/plex-library/plex-facets/
plex-collections/plex-playlists all 200, full UI renders.
Introduce lib/queryKeys.ts as the single source of truth for React Query
keys — the R7 driver was ~200 hand-written key literals across 51 files
(`["feed"]` alone in 15+), where one typo silently orphans a key so an
invalidate never matches.
S2a migrates the three highest-traffic domains (feed/video, channels,
playlists) plus the shared me/my-status/tags roots: ~90 literal sites in
18 files now call qk.*(). Pure substitution — each factory fn returns the
byte-identical array the call sites used before, so TanStack's prefix
matching is unchanged (qk.feed() still invalidates qk.feed(filters)).
Co-invalidation clusters are deliberately NOT bundled into helpers (they
differ per site), so no query's refetch scope changes.
Nullable keys (playlist(selectedId), ytSearch(q)) accept null so a null
segment is preserved (["playlist", null]) while a no-arg call is the bare
prefix key — guard is `!== undefined`.
Gate green: typecheck, eslint (0 err), prettier, 68 vitest. Smoke: app
boots, feed/facets/tags/my-status all 200, no console errors. Factory
extended to plex/messaging/admin/downloads/notifications in S2b.
- localizeDetail: don't leak FastAPI's snake_case `loc` field names into a
translated 422 toast; return a clean, fully localized validation message
(drops the loc filter/pop parsing entirely)
- me(): drop the dead `r &&` guard now that req<T> types r non-nullable
req() returned Promise<any>, so every api.* return annotation was an
unchecked assertion and the 17 action endpoints leaked `any` to callers.
Make req generic with a single audited cast at the boundary; T flows from
each method's declared return type via contextual inference, so no call
site needs a type arg except where none existed:
- me(): type the /api/me probe ({ authenticated } + Partial<Me>)
- deepAll / syncSubscriptions: real backend-verified response shapes
- HttpError.detailData, localizeDetail(d), inner detailData: any -> unknown
Also teach localizeDetail the FastAPI 422 detail-ARRAY shape (carried in
from R6 S1b review): now that malformed requests return 422, surface the
offending field via a localized errors.validation key instead of falling
through to the generic toast.
No runtime change on normal flows (req is await-identical); the 422 branch
only fires on genuinely malformed input. Gate green: typecheck, eslint,
prettier, 68 vitest. Smoke-tested localdev: /api/me + feed 200, no console
errors.
Backend-heavy, frontend-invisible hardening over four sprints, all user-triggered
/code-review clean + UAT-passed on localdev:
- S1a: shared iso()/owned_or_404/require_write_scope helpers.
- S1b: all ~52 payload:dict endpoints → Pydantic request models (3 named prod-500s
on garbage input become clean 422s); status-code unification.
- S3 (pulled ahead of S2): DB-backed pytest lane (ephemeral Postgres, alembic head)
wired into the gate; /facets genre-null regression; migration 0060 normalises
existing JSONB-null plex rows.
- S2: quota accounting owns its session (durable across a caller rollback, never
commits the caller); Plex sync commits per-section. Proven by DB-lane tests.
Suite 159 -> 181 (incl. the new DB lane). db_revision 0060. No response-shape change.
/code-review high on S2 — 4 findings, 2 fixed, 2 no-change:
- test_plex_sync_txn: a faked PlexClient fails on the second library; asserts the
first library's row survives (per-section commit). Mutation-checked — moving the
commit back out of the loop turns it red ([] == ['1']).
- dropped the `[[quota-owns-its-session]]` memory slugs from quota.py docstrings
(a dangling private-note reference in shipped code; the reasoning is already inline).
No change: record_usage's per-call second connection is a transient spike, fine for
this app's concurrency; the two-site `with SessionLocal()` duplication isn't worth a
helper until a third caller appears.
Gate: ruff clean; DB lane 181 green.
Transaction ownership (C-A5):
- quota.record_usage / log_action write in their OWN SessionLocal(), committed
immediately, and no longer take/commit the caller's db. The spend stays DURABLE
regardless of the caller (read-only YouTube paths never commit; a failed job rolls
back) — under-counting the shared daily budget would risk overspending the real
YouTube quota — AND quota stops flushing a job's partial state mid-run behind its
own `except: db.rollback()`. units_used_today switches to a scalar SELECT so
measured()'s before/after diff sees the separate session's commits (READ COMMITTED).
Call sites updated (youtube/client.py ×3, routes/search.py ×1).
- plex sync commits PER SECTION (moved the end-of-loop commit inside), so a PlexError
on a later library keeps the ones already mirrored (idempotent; next run reconciles).
The sync jobs did NOT rely on record_usage's incidental commit for persistence — their
per-item functions (apply_rss_feed, backfill_channel_recent, import_subscriptions,
sync_user_playlists, apply_channel_autotags) all commit internally, so the in-loop
rollback handlers already isolate a failed item. No loop restructuring needed.
DB-lane tests (test_quota_ownership, 4): spend survives a caller rollback while the
caller's uncommitted row does NOT leak; accumulation; measured-style mid-txn read.
Gate: ruff clean; DB lane 180 green.
/code-review high on S3 — 3 low findings, 2 fixed, 1 no-change:
- _db_reachable() now probes via a throwaway engine with connect_timeout=3, so a
slow/firewalled DATABASE_URL can't hang the whole suite at import (the app engine
has no connect timeout).
- test_plex_facets sets the "configured" gate flag via monkeypatch (auto-restored)
instead of poking state._configured_cache in a manual finally — no leak risk, and
the now-unnecessary mark_configured DB write is dropped.
No change: test_plex_jsonb_normalize copying migration 0060's SQL is deliberate —
migrations must stay self-contained (no app imports) and a shipped migration is
immutable, so there's no real drift to guard against.
Gate: ruff clean; DB lane 176 passed; pure-logic 172 passed / 4 skipped (fast).
First real customers of the DB lane:
- test_plex_facets: GET /api/plex/facets?scope=show returns 200 (not the
"cannot extract elements from a scalar" 500) for a show whose genres is a JSONB
scalar `null` — the 0.53.1 read-guard's regression test. Mutation-checked:
removing the jsonb_typeof='array' guard turns it red with that exact error.
- migration 0060 (S3a): one-time UPDATE …=NULL WHERE jsonb_typeof=… 'null' over the
plex_items/plex_shows tag columns, so pre-0.53.1 JSONB-null rows become uniform SQL
NULL and the read-guards go belt-only. test_plex_jsonb_normalize verifies scalar-null
→ SQL NULL while a real array is preserved.
Gate: ruff clean; DB lane 176 passed; pure-logic lane unchanged (skips the DB tests).
The suite was pure-logic only (R2 S2b deferred the DB lane). Add an opt-in `db`
fixture (tests/conftest.py) that hands a test a real session against an ephemeral
Postgres; schema is built with `alembic upgrade head` (the real prod schema needs
the unaccent extension + unaccent_simple TS config that create_all can't provide,
and this also proves the migration chain applies). Each test starts from a TRUNCATEd
clean DB so tests that COMMIT — the whole point, for R6 S2's transaction work —
don't leak.
backend/docker-compose.test.yml wires an ephemeral tmpfs pg + the test image. When
no Postgres is reachable (a plain `docker run`), the DB tests SKIP and the pure-logic
lane is unchanged: 172 passed, 2 skipped. With the DB: 174 passed.
test_db_lane.py smoke-proves commit round-trip + TRUNCATE isolation. The gate wiring
(siftlode.sh) lands in the ops repo.
/code-review high round 1 on S1b — 5 findings, 1 fixed, 4 by-design/deferred:
- FIXED: VideoProgressIn / ItemProgressIn position_seconds & duration_seconds
typed `int` would 422 a raw float currentTime on the 10s progress checkpoint
(the old `int(payload.get(...))` truncated it). Back to `float`, truncated in
the handler; +test. The live frontend floors so this was latent, not live.
By-design / deferred (no change): the 422 detail-array isn't localized by the FE
(filed under R7); PATCH is-not-none (channels/quota, null-safe) vs model_fields_set
(nullable-clear) is a deliberate split; missing-field→422 is the point of the epic;
me/switch_account's numeric-string coercion is more correct and still session-gated.
Gate: ruff clean, pytest 172 green.
Pure-logic tests that the three ROADMAP-named 500-producers (channel priority,
tag_id, setup password) and admin quota now reject bad input at the model
boundary, and that the PATCH models separate absent from explicit-null via
model_fields_set. Mutation-checked: reverting a field type turns them red.
All 11 download endpoints off payload:dict. Highlights:
- admin_set_quota: AdminSetQuotaIn — the `int(payload["max_bytes"])` 500 on a
non-number is now a 422; each field set only when provided (is not None).
- profiles/enqueue/edit: CreateProfileIn / UpdateProfileIn / EnqueueDownloadIn /
EnqueueEditIn; spec/edit_spec typed as dict (non-object → 422). _resolve_spec now
takes (profile_id, spec) instead of the raw payload.
- meta/share/links: UpdateDownloadMetaIn / ShareDownloadIn / CreateLinkIn /
UpdateLinkIn; _link_expiry takes an int|None instead of digging the payload.
PATCH endpoints read model_fields_set so an explicit null still clears a field.
Gate: ruff clean, pytest 164 green.
CreatePlaylistIn / RenamePlaylistIn (PATCH via model_fields_set) / ReorderItemsIn,
plus a shared VideoRefIn(video_id) for both add-video endpoints (watch-later +
add-item). A non-string video_id is now a 422 instead of reaching db.get.
Gate: ruff clean.
- setup: SetupAdminIn (KILLS the 3rd named 500 — a non-string password reaching
argon2/len()), SetupTestEmailIn; setup_config stays a typed dict[str, Any] (a
genuinely dynamic key→value settings map, not a fixed schema).
- admin: AddInviteIn / SetRoleIn / SetSuspendedIn / AddDemoWhitelistIn. The
role/status business-rule 400s are unchanged; type errors are now 422.
All 3 ROADMAP-named 500-producers now fixed (channels priority + tag_id, setup
password). Gate: ruff clean, pytest 164 green.
- saved_views: CreateViewIn / UpdateViewIn / ReorderViewsIn. PATCH presence via
model_fields_set; filters typed as dict (a non-object is now 422, an explicit
null on update stays a 400 "filters must be an object").
- feed: VideoStateIn(status) / VideoProgressIn(position_seconds,duration_seconds) —
the manual int()+400 in progress becomes a 422 on a non-number; the status
membership 400 and the video 404 are unchanged.
Gate: ruff clean.
update_channel: payload:dict → ChannelUpdateIn (priority/hidden/deep_requested,
all optional for PATCH semantics). Kills the `int(payload["priority"])` 500 the
prod report hit — a non-numeric/garbage value now returns 422, not 500.
attach_tag: payload:dict → AttachTagIn(tag_id:int) — a non-int tag_id was a 500
in `db.get(Tag, ...)`; the personal-tag ownership check reuses owned_or_404.
Round-1 review finding #4 named only audit.py/plex.py, but the same misplaced
`from app.utils import iso` sat in channels/downloads/feed/notifications too — the
round missed them (ruff enforces no import order here, so the gate was silent).
Move them to alphabetical position for consistency. No behaviour change.
/code-review high round 1 (5 findings):
- iso(dt) now typed `date | None` — MediaAsset.upload_date is a plain `date`, not a
datetime; the helper's signature was too narrow for the type-contract epic. +test.
- restore alphabetical app.* import order in audit.py and plex.py (the new iso/_common
imports had been dropped in mid-block).
The other 3 findings are deliberate design choices, left as-is for UAT: push/push-plan
now gate write-scope before the 404 (dependency ordering); the unified 403 message; and
owned_or_404's owner_attr kept for S1b's DownloadShare.to_user_id-style owners.
Gate: ruff clean, pytest 164 green.
Establish the R6 API-contract infra before the S1b payload:dict migration:
- iso(dt) in utils.py replaces the ~16 hand-rolled `x.isoformat() if x else None`
serializer idioms across the routes.
- get_or_404 / owned_or_404 in routes/_common.py — one 404 + existence-leak policy;
plex's _own_playlist_or_404 and saved_views' _own_view now delegate to it.
- require_write_scope dependency in auth.py collapses the inline has_write_scope→403
checks (channels subscribe/unsubscribe, playlists push/push-plan) that had drifted
into three different messages into one; the conditional delete_playlist guard stays
inline (its message was already canonical).
No response-shape change (iso == isoformat for a present value). Gate: ruff clean,
pytest 163 green (+ test_utils for iso).
justify-center + overflow-x-auto stranded the left-end controls (Previous, the
N/M counter) when the bar overflowed a narrow window. justify-start makes the
overflow scroll from the start — and matches the conventional left-aligned
video-player control bar.
The overlaid, auto-hiding bar fought YouTube's seek bar for the same bottom
pixels: while ours showed it covered YT's seek + buttons, and reaching them
meant moving off the area and waiting for our bar to fade — a fiddly, "random"-
feeling dance keyed on mouse movement.
Now the bar is DOCKED and always visible: the video area is inset by the bar's
height (a wrapper around the mount, since YT.Player replaces the mount element
with its iframe and would drop a class set on it), so the bar sits in its own
strip and never overlaps the video. YouTube's seek/controls stay reachable by
hovering the video's bottom edge, just above ours. No auto-hide, no guessing,
no fight. Verified in Chrome: iframe.bottom == bar.top (no overlap), bar opacity 1.
UAT feedback on the F-maximise view — three fixes:
- Own control bar in the big views (maximise + our fullscreen): previous/next +
N/M, auto-advance, loop, add-to-playlist, download, mark-watched, exit, and a
true-fullscreen toggle. Auto-hides on idle. Add/download are hidden in true
fullscreen (their popovers portal to <body>, outside the fullscreen element).
- Wheel volume works in the big views again: the centre interaction overlay is
now armed in maximise/fullscreen (it only yields to YouTube's own settings
menu), so the wheel bubbles to our handler instead of the bare iframe. F also
offers true fullscreen that WE own (Fullscreen API on the stage), so wheel/
arrow volume and our controls work inside it too — YouTube's own fullscreen
button, which we never surface, is not involved.
- Back (mouse/browser) steps out one level at a time via nested history layers
(useBackToExit): fullscreen -> maximise -> the modal -> closed, instead of
closing the whole player at once.
Verified in Chrome: wheel volume in maximise + fullscreen, the control bar, and
the full Back ladder (fullscreen -> maximise -> modal -> closed).
A mirrored Shows library whose items have no genres stored `genres` as JSON
`null` — SQLAlchemy's JSONB persists a Python None as a JSON scalar, not SQL
NULL. /facets unnested it with jsonb_array_elements_text(), which raises
"cannot extract elements from a scalar", so a single bare library 500'd the
whole Plex module on open (the grid loads, the filter sidebar's facets call dies).
- Guard the /facets genre unnest with jsonb_typeof(genres)='array', mirroring
the guard people.py already applies to the same columns.
- Persist the JSONB tag columns (genres/directors/cast_names/collection_keys on
plex_items and plex_shows) with none_as_null=True, so an empty list round-trips
as SQL NULL and self-heals on the next sync — closing the trap for any future
unnest.
Verified against the real prod row (DumaTV show, genres = JSON null): the
unguarded query errors, the guarded one returns cleanly.
Found by running the round-8 fix in the real container instead of trusting the
unit test: a directory created 2ms AFTER `_PROCESS_START` was stamped with an
mtime 2ms BEFORE it, so the sweep deleted it as a leftover. The two sides come
from different clocks — a file's mtime from the kernel's coarse clock (one timer
tick of lag), `time.time()` from the fine one — and the unit test could not see it
because it supplies an artificial cutoff.
The window is small and lands exactly where it hurts: the sweep runs at boot, so
the session at risk is one started in the first instants after startup — the case
the guard exists for. Keep-side slack of 2s; the cost is that a leftover from the
last seconds before a restart survives until the next one.
Verified in the container: a backdated leftover is swept, a directory created by
this run is kept, and a registered live session with an old mtime is kept — 1 of 3.
Test pins the skew (mutation-checked: with the grace at 0 it goes red).
The worst one was, again, the previous round's own fix.
- Round 7 stopped awaiting the HLS sweep so startup could serve immediately — which
also made it run CONCURRENTLY with playback, and `sweep_orphans` snapshotted the
live-session set BEFORE listing the root and rmtree'd outside the lock. Since
`start_session` reuses a directory path for the same (key, offset, tag), a stale
name can become a live session at any moment. Two guards now: nothing whose mtime
is newer than this process's start is ever deleted, and the live check + rmtree
happen together under the lock, one directory at a time.
- The sweep task is awaited after cancel, so shutdown can't leave it pending.
- `_rate_limiter` used a truthiness test on its timestamp, so a `now()` of exactly
0.0 read as "never ran" — unreachable in production, but `now` exists to be
injected and a test clock starting at 0 is the obvious choice.
- The RSS total-outage error now carries the first underlying error: "all N channels
failed" is also what ONE dead feed id looks like on a small instance, and a red
card the user can't act on is its own kind of dishonest.
- The WebKit fullscreen handling moved into `lib/fullscreen` and PlexPlayer uses it
too — it had the same unprefixed-only reads in three places, including an Escape
branch that would have closed the whole player instead of leaving fullscreen.
- The `--max` CSS comment claimed `inset: 0` did the sizing; Tailwind's `w-full` is
still on the element and wins. Comment now states the real mechanism.
Verified in real Chrome (the pane can't composite or do native fullscreen):
- F fills the 1920x889 viewport; the exit pill renders top-right on the letterbox
band and overlaps NO YouTube control — the collision this round suspected does not
reproduce in this embed (its own buttons sit bottom-left / bottom-right).
- Four real ArrowDown presses while maximised: persisted volume 100 -> 80, with the
level flash visible on screen.
- A real click on the exit pill un-maximises and returns focus to the card.
- lib/fullscreen driven from a real click: enter (innerHeight 889 -> 1024), the
change listener fires exactly once per transition, exit returns to 889.
Backend suite 150 -> 158; the new sweep tests are mutation-checked, and they read
their cutoff from the filesystem rather than a clock (the container VM and the
bind-mounted host disagree by enough to make a clock-based assertion flaky).
Five of them sit where round 6 turned F from a Fullscreen API call into a CSS
maximise: the guards around it still asked `isFullscreen`, which that pivot made
permanently false.
- Arrow-key volume was dead in the maximised view: the scroll-deference checked
only `isFullscreenRef`, so the (now hidden but still scrollable) card kept
swallowing Up/Down. Both flags now count as "the video fills the screen".
Measured: 87px of scroll room on the focused card, ArrowDown moves 80 -> 75
and scrolls nothing.
- The maximised view had no way out once the pointer touched the video. Reaching
YouTube's controls means clicking into the cross-origin iframe, which takes
keyboard focus with it, and the re-arm paths (stage mouseleave, window focus)
need the pointer to leave the browser entirely while the stage fills it. So
the exit now lives on screen, in our DOM: a button that un-maximises AND pulls
focus back, re-arming every shortcut.
- WebKit fires only `webkitfullscreenchange`/`webkitFullscreenElement`, so on
Safari/iPadOS YouTube's own fullscreen never registered — no overlay yield, no
focus reclaim. One `fullscreenEl()` reader, both event spellings.
- The stage claimed `z-index: 9999`, the glass tuner's reserved level, for a job
that only needs to beat the modal chrome it is nested in (its siblings top out
at z-menu). Dropped to rail-level; nothing escapes the modal's stacking
context either way.
- `inset: 0` already sizes a fixed layer — the inherited `100vw`/`100vh`
over-constrained it, which is how a classic scrollbar gets a horizontal
overflow and a mobile URL bar hides the bottom edge (YouTube's control bar).
- The shortcut hint still promised "F: fullscreen" in both locales.
Backend, both "the fix stopped one layer short":
- The startup HLS sweep was awaited inside lifespan, so uvicorn still served
nothing until it finished — a thread doesn't help when nothing is listening
yet. Fire-and-forget task instead, cancelled on shutdown. Measured with 200
planted segments: "Application startup complete" now precedes "swept 1".
- An RSS poll where every channel failed returned normally, so the scheduler
recorded `ok` with the outage buried in the summary string. `failed == total`
now raises: a run that reached nothing is a failed run.
Suites: backend 147 -> 150.
UAT found a dead end: F -> YouTube's fullscreen button -> Esc left the player
maximised with no way back, and a further F produced a second, control-less
fullscreen with the wheel driving YouTube's own volume box.
Measured cause: reaching YouTube's control bar means clicking INTO the player,
which moves keyboard focus into the cross-origin iframe — and it stays there.
Our window-level shortcuts then never fire (document.activeElement === IFRAME),
so F went to YouTube instead of toggling our maximise.
Fix: on fullscreenchange, when fullscreen ends, focus the modal card again.
Verified in real Chrome, step by step:
- F -> maximised, no browser lock, focus on our card
- YouTube's own FS button -> real fullscreen (innerHeight 889 -> 1024)
- leaving fullscreen -> focus back on our card (was: still the iframe)
- F -> back to the normal modal view (was: a second fullscreen with no controls)
Root cause, measured in Chrome/Brave/Firefox alike (so: spec behaviour, not a
browser bug): a cross-origin embed reads its fullscreen state from ITS OWN
document. While we hold the browser's fullscreen lock — on our wrapper OR on the
iframe element — the YouTube player never learns it is fullscreen, so its own
fullscreen button stays in the "enter" state and clicking it changes nothing
visible. Every other YouTube control works, which is exactly what the user saw.
So we stop taking the lock: F now toggles a CSS layer (`player-stage--max`) that
fills the viewport. The page stays non-fullscreen, so the embed's own fullscreen
button behaves natively, and our own overlays and shortcuts keep working because
our DOM is what's on screen. Escape steps out of maximise before closing.
Verified end to end in real Chrome:
- F -> stage 1920x889, document.fullscreenElement null (no lock taken)
- YouTube's own fullscreen button -> real fullscreen (innerHeight 889 -> 1024,
fullscreenElement becomes the IFRAME) and its icon flips to "exit"
- clicking it again -> back to the maximised view, our class still applied
- wheel volume while maximised: 100 -> 85, with the level flash visible
Drops the "YouTube fullscreen" toolbar button added earlier today: with the lock
gone, YouTube's own button does that job correctly.
The reported bug: after F, the embed's own fullscreen button is dead. Root cause
measured in Chrome — the browser's fullscreen lock sits on OUR wrapper, so
YouTube's button would need a nested request from a cross-origin frame. Handing
the lock to the iframe fixes it, but `iframe.requestFullscreen()` only resolves
from a CLICK (from a key press the promise stays pending forever, and awaiting
it spends the activation so a fallback is denied too).
So: F keeps fullscreening our wrapper (unchanged, our overlays stay visible) and
a new toolbar button hands the lock to the iframe — verified in real Chrome:
document.fullscreenElement is the IFRAME at 1920x1024, i.e. a plain YouTube
fullscreen where its own controls work.
Round-6 review findings, all fixed:
- one _rate_limiter + one _cancel_poll shared by the yt-dlp hook and run_ffmpeg
(was three hand-rolled throttles; cancel latency differed 2s vs 1s)
- stepVolume takes {delta, fallback} — two adjacent number args were silently
transposable
- volume gestures before the player is ready are honoured instead of dropped
- normalizeVolume's fallback is typed unknown (its guard was unreachable) and
applyVolume takes a number again
- the throttle tests inject a clock instead of patching global time.monotonic
- run_ffmpeg stops the process from ONE exit path (`except BaseException`), so
an exception out of on_progress (a DB write) can no longer orphan a live
ffmpeg after a 10s join on its pipes; _stop is now a no-op on a dead process
- both run_ffmpeg callbacks are throttled to ~1s (they were a DB round-trip per
output line: ~10^5 queries on a long re-encode), mirroring the download hook
- volume arithmetic extracted to lib/playerVolume with unit tests; a corrupt
level now falls back to the LAST GOOD one instead of full blast, and the
sanitising happens where the stored value enters
- RSS is back to two except clauses (the shared tail already sits after the try)
- new tests: callback-exception cleanup, the two throttles; the fake Popen grew
poll() and its wait count is now asserted
Mutation-checked: each new group goes red when its fix is reverted.
- Feed memoizes onClose (like Playlists already did): the inline closure re-ran
PlayerModal's keydown effect on every Feed render, which since the debounce
landed also flushed the volume write and re-stole focus
- the player reads its volume with readAccountMerged again — a partial stored
object made the level undefined -> NaN -> a persisted null; applyVolume also
refuses a non-finite level outright
- that read moved into a lazy useState initializer (was running per render)
- the two RSS except branches share one tail, so a counter can't drift again
- run_ffmpeg tests cover the SIGTERM->SIGKILL escalation and the lingering-
process path; dropped an assertion that only restated the fixture
- storage.ts documents the actual { volume } shape (0 IS muted)
- the sweep guard anchors the rating_key to digits: the round-2 widening had
degenerated to "anything ending in _<digits>" (backup_2024, release_10)
- RSS counts apply-side failures too, so a read-only database no longer reports
a wholly failed poll as "ok — new=0, failed=0"
- the player reads/writes its volume directly (readAccount/writeAccount): the
persisted-object hook rendered nothing and cost a second write per settle
- new tests: the sweep guard's accept/reject table, and run_ffmpeg's threading
(drain, cancel while silent, stall watchdog, stderr tail on a nonzero exit)
- the breaker sentinel is checked one way (isinstance) at all four sites
- run_rss_poll is typed dict[str, int]
All three new test groups were mutation-checked: reverting each fix turns them
red (or, for the cancel/stall pair, hangs — which is the bug itself).
- the volume keys no longer defer to a scrollable card while in fullscreen
(mirrored into a ref: the keydown handler is bound once and state would be stale)
- the debounced volume flush writes straight to storage; persisting inside a
setState updater is not guaranteed to run for an unmounting component
- the HLS sweep also matches the original {key}_{start} directory naming, which
is what the oldest leftovers on disk actually use
- run_rss_poll returns {new, failed} so an egress outage stops rendering as
"ok — 0" in the scheduler card and the audit log; /api/sync/rss reports it too
- drop the unused poll_rss_channel (it silently gained a new exception)
- _Breaker.run is generic, so a call site keeps its item-id type
- test helper instead of the generator-throw idiom
- 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
- the interaction overlay yields fully in fullscreen: YouTube only reveals its
control bar while the iframe itself sees the mouse, so after pressing F the
native fullscreen button was unreachable
- remember the volume per account (a YT.Player is created per video and always
starts at 100), plus ArrowUp/ArrowDown volume keys that work in fullscreen
- Ctrl/Cmd+wheel falls through to the browser's zoom instead of the volume
ffprobe prints codec_name BEFORE codec_type, so the line parser attributed every
name to the previous stream: a plain H.264+AAC mp4 probed as (aac, None) and the
browser-playability check re-encoded a perfectly good file on EVERY download.
Verified on localdev — the same source now stores without a re-encode pass.
- never prune subscriptions on an empty fetch: one anomalous empty-but-200
response wiped every subscription (and its deep_requested flag) in one run
- push_playlist: stop after 5 consecutive write failures — a quota-dead or
scope-revoked account otherwise burns 50 units per remaining item
- RSS: raise RssError instead of returning [] on a transport/HTTP failure, so a
failed poll no longer stamps last_rss_at and a 404 feed stops looking healthy
- worker: exit when the schema never appears instead of "continuing" into a
crash one call later
- pick the produced file safely: Path("") is Path("."), which passes .exists(),
so a missing yt-dlp filepath skipped the staging fallback and crashed in
with_name ("PosixPath('.') has an empty name") on non-YouTube sources
- unwrap a playlist/multi_video extraction to the entry that carries the media
- run_ffmpeg: drain stderr while it runs (a full 64KB pipe blocked ffmpeg
forever and hung the worker thread), 1s cancel polling, stall watchdog and a
bounded wait on exit
- escape apostrophes/backslashes in the ffmpeg concat list (pre-0.51.0 names)
- sweep orphaned HLS segment dirs at startup (nothing else ever reaped them)
Review follow-up on the share-link password UI:
- Removing a link's password makes it publicly watchable, a protection
downgrade; gate it behind a danger-confirm like the neighbouring Revoke,
spelling out that anyone with the URL can then watch without a password.
- When setting a first password the input reused the create-form "Password
(optional)" placeholder, but it's required here (Save stays disabled until
non-empty); use the "Set a password" wording instead.
peter