diff --git a/CHANGELOG.md b/CHANGELOG.md index 3f4efffd..89b34da5 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -7,6 +7,17 @@ edit the release notes and re-run the generator. Upgrading, pinning a version and rolling back: [docs/self-hosting.md](docs/self-hosting.md#upgrading). +## 0.114.0 — 2026-10-29 + +### New + +- Notifications: what asks for you comes first, then everything else as one stream, newest first. Read notices older than a week fold under Earlier, and Back returns to the row you left. + +### Fixed + +- Plex: a progress save sent just before an episode ends no longer leaves it unwatched, and a refused finish no longer sticks. +- After a dialog closes, the keyboard focus returns to where it was, also inside a menu that stays open. + ## 0.113.0 — 2026-10-28 ### New diff --git a/VERSION b/VERSION index 5fea1768..18455b77 100644 --- a/VERSION +++ b/VERSION @@ -1 +1 @@ -0.113.0 +0.114.0 diff --git a/backend/app/plex/service.py b/backend/app/plex/service.py index 215303e7..b090166d 100644 --- a/backend/app/plex/service.py +++ b/backend/app/plex/service.py @@ -6,9 +6,9 @@ The rest of the mirror's writes stay with the sync (`plex.sync`): a collection's after a write-back, its local cleanup after a delete. """ -from datetime import datetime, timezone +from datetime import datetime, timedelta, timezone -from sqlalchemy import CursorResult, delete, func, select, update +from sqlalchemy import CursorResult, delete, func, or_, select, update from sqlalchemy.dialects.postgresql import insert as pg_insert from sqlalchemy.orm import Session @@ -271,6 +271,10 @@ def reorder_playlist(db: Session, pl: PlexPlaylist, rating_keys: list[str]) -> N # --- a user's watch state -------------------------------------------------------------------- +# How long after a watched mark a mid-item position is read as the finishing player's late tick +# (`checkpoint`): the tick is every 10 s, plus the request's own latency and a retry. +_LATE_CHECKPOINT_S = 30 + def mark_finished(db: Session, user_id: int, item_id: int, now: datetime) -> bool: """Mark the item watched (position cleared) and say whether THIS call made it so. Two near-end @@ -352,16 +356,33 @@ def checkpoint( now: datetime, *, dirty: bool, -) -> PlexState: - """Record a resume position. `dirty` marks it not yet mirrored to Plex — set before a push, so - the reconcile re-pushes it if the push fails and the push can settle it once Plex confirms.""" +) -> bool: + """Record a resume position, and say whether it was recorded. `dirty` marks it not yet mirrored + to Plex — set before a push, so the reconcile re-pushes it if the push fails and the push can + settle it once Plex confirms. + + Not onto a row marked watched in the last `_LATE_CHECKPOINT_S`: that position is the player's + tick sent just before its own finish (or a mark from another tab), and writing it made a watched + item in progress again — on Plex too, through the resume push it carried. Decided by the write + itself, since the mark can commit between any read here and the write. A rewatch is checkpointed + as before once the mark is that far behind it.""" st = state_row(db, user, item_id, st) - st.position_seconds = position - st.progress_updated_at = now + values: dict[str, object] = {"position_seconds": position, "progress_updated_at": now} if dirty: - st.synced_to_plex = False + values["synced_to_plex"] = False + not_just_finished = or_( + PlexState.status != "watched", + PlexState.watched_at.is_(None), + PlexState.watched_at <= now - timedelta(seconds=_LATE_CHECKPOINT_S), + ) + result = db.execute( + update(PlexState) + .where(PlexState.id == st.id, not_just_finished) + .values(values) + .execution_options(synchronize_session="fetch") + ) db.flush() - return st + return isinstance(result, CursorResult) and result.rowcount > 0 def forget_state(db: Session, st: PlexState) -> None: diff --git a/backend/app/plex/stream.py b/backend/app/plex/stream.py index f273710d..7c4ec00a 100644 --- a/backend/app/plex/stream.py +++ b/backend/app/plex/stream.py @@ -66,7 +66,9 @@ _SESSION_IDLE_S = 600 # reap a session with no access for this long # restart leaves them behind like any other. `sweep_orphans` deletes ONLY names # matching one of those two shapes: `_HLS_ROOT` is admin-configurable (PLEX_HLS_DIR), so it may # legitimately point at a shared scratch path we do not own, and a blanket "delete every directory -# under the root" would take the neighbours with it. +# under the root" would take the neighbours with it. A torn-down session's directory is renamed to +# its own name + `_TOMB` + 8 hex digits before it is deleted (see `_detach`), so a crash between the +# two leaves a name this sweep still recognises. # # The KEY is anchored to digits (a Plex rating_key is a number) — that is what keeps foreign names # out. A `.+` there let `snapshot_2024_backup_+1.00` through, and pairing `.+` with an optional tail @@ -74,8 +76,10 @@ _SESSION_IDLE_S = 600 # reap a session with no access for this long # non-numeric rating_key would merely leave its directory unswept, which is the safe direction to # fail. Covered by test_plex_stream.py — this regex is the only thing standing between the sweep # and someone else's data. +_TOMB = ".dead-" _SESSION_DIR_RE = re.compile( - r"^\d+_\d+(_[^_]+_[+-]\d+\.\d{2})?$|^\d+_u\d+_\d+_[^_]+_[+-]\d+\.\d{2}$" + r"^\d+_\d+(_[^_]+_[+-]\d+\.\d{2})?$" + rf"|^\d+_u\d+_\d+_[^_]+_[+-]\d+\.\d{{2}}({re.escape(_TOMB)}[0-9a-f]{{8}})?$" ) # When this process started. `sweep_orphans` only deletes directories older than this: everything # newer belongs to a session THIS run created, and since `start_session` reuses a directory path @@ -93,7 +97,7 @@ _MTIME_GRACE_S = 2.0 _lock = threading.Lock() # Keyed by VIEWER and item, (user_id, rating_key). Keyed by the item alone, a second account opening -# the same film `_kill`ed the first one's session: the displaced viewer got a 404, reclaimed at its +# the same film tore down the first one's session: the displaced viewer got a 404, reclaimed at its # playhead, and so evicted the other in turn — two people could not watch one film at once. (One # account on two devices still shares a session; that case ends in the player's bounded reclaim.) _sessions: dict[tuple[int, str], "HlsSession"] = {} @@ -641,29 +645,82 @@ def _probe_first_pts(seg: Path) -> float | None: return None -def _kill(s: HlsSession) -> None: +# Teardown is split in two, because every segment request and checkpoint takes `_lock`: waiting up to +# 3 s for an ffmpeg to exit and deleting a whole segmented film under it stalled every other +# viewer's playback for one viewer's seek or one reap pass. `_detach` (under the lock) signals the +# process and renames its directory aside — so the deletion can never hit a NEW session that reuses +# the path — and `_finish` (after the lock is released) waits and deletes. +# +# What the rename cannot cover is the dying ffmpeg itself: it writes by path, so until it has exited +# it can still put a segment, or its final ENDLIST playlist, into a new session's directory on the +# same path. `_dying` remembers it by that path, and `_spawn` waits it out — under the lock, on +# purpose — before it creates the directory again. +_dying: dict[Path, subprocess.Popen] = {} + + +def _await_exit(key: str, proc: subprocess.Popen) -> None: try: - s.proc.terminate() try: - s.proc.wait(timeout=3) + proc.wait(timeout=3) # silent-ok: the escalation IS the handling. A transcode that ignores TERM for three # seconds gets KILL, which is exactly what this branch does. except subprocess.TimeoutExpired: - s.proc.kill() + proc.kill() except Exception as exc: # noqa: BLE001 — teardown finishes whatever the process does # Not nothing: what survives this is an ffmpeg nobody owns any more, holding a VAAPI # handle and a core, and the transcode cap that is supposed to bound them counts rows in # `_sessions` rather than processes. One line is the only way that ever surfaces. + log.info( + "could not stop the transcode process", + extra=logs.event("plex.stream.kill_failed", key=key, reason=type(exc).__name__), + ) + + +def _detach(s: HlsSession) -> tuple[HlsSession, Path | None]: + """Caller holds _lock and has taken `s` out of `_sessions`. Hand the result to `_finish` once the + lock is released.""" + try: + s.proc.terminate() + except Exception as exc: # noqa: BLE001 — teardown finishes whatever the process does log.info( "could not stop the transcode process", extra=logs.event("plex.stream.kill_failed", key=s.key, reason=type(exc).__name__), ) - shutil.rmtree(s.dir, ignore_errors=True) + _dying[s.dir] = s.proc + tomb = s.dir.with_name(f"{s.dir.name}{_TOMB}{secrets.token_hex(4)}") + try: + s.dir.rename(tomb) + # silent-ok: nothing on disk (ffmpeg died before writing, or a sweep took it) — nothing to delete. + except FileNotFoundError: + return s, None + except OSError as exc: + # Cannot be moved aside, so it cannot be deleted outside the lock either: the path may be a + # new session's a moment from now. Deleted here instead, the old way. + log.info( + "a session directory could not be moved aside; deleted under the lock", + extra=logs.event("plex.stream.tomb_failed", key=s.key, reason=type(exc).__name__), + ) + shutil.rmtree(s.dir, ignore_errors=True) + return s, None + return s, tomb -def _enforce_caps(user_id: int, max_transcodes: int, max_sessions: int) -> None: - """Caller holds _lock. Three bounds, applied after a session is registered, each dropping the - least recently accessed sessions over it: +def _finish(doomed: "list[tuple[HlsSession, Path | None]]") -> None: + """The slow half of a teardown, called WITHOUT `_lock`.""" + for s, tomb in doomed: + _await_exit(s.key, s.proc) + with _lock: + if _dying.get(s.dir) is s.proc: + del _dying[s.dir] + if tomb is not None: + shutil.rmtree(tomb, ignore_errors=True) + + +def _enforce_caps( + user_id: int, max_transcodes: int, max_sessions: int +) -> "list[tuple[HlsSession, Path | None]]": + """Caller holds _lock, and `_finish`es what this returns once it has released it. Three bounds, + applied after a session is registered, each dropping the least recently accessed sessions over it: * this account's own sessions (`_MAX_SESSIONS_PER_USER`) — first, so a single account filling the box evicts itself before anyone else; @@ -676,6 +733,8 @@ def _enforce_caps(user_id: int, max_transcodes: int, max_sessions: int) -> None: and the player restarts at the playhead, so from the server nothing failed — and "my film keeps rebuffering" has no other explanation anywhere, hence the line.""" + doomed: list[tuple[HlsSession, Path | None]] = [] + def cull(pool: list[tuple[tuple[int, str], HlsSession]], cap: int, bound: str) -> None: if len(pool) <= cap: return @@ -690,12 +749,13 @@ def _enforce_caps(user_id: int, max_transcodes: int, max_sessions: int) -> None: sessions=len(pool), ), ) - _kill(sess) _sessions.pop(reg, None) + doomed.append(_detach(sess)) cull([kv for kv in _sessions.items() if kv[0][0] == user_id], _MAX_SESSIONS_PER_USER, "user") cull([kv for kv in _sessions.items() if kv[1].transcode], max_transcodes, "transcode") cull(list(_sessions.items()), max_sessions, "total") + return doomed def start_session( @@ -759,68 +819,79 @@ def start_session( # can't starve the co-located API/playback. preexec_fn runs in the forked child (POSIX); on a # platform without fork (the API always runs in a Linux container in prod) it's simply omitted. preexec = _sw_child_preexec if (transcode and not hw and os.name == "posix") else None - with _lock: - if replace_only is not None and _sessions.get(reg) is not replace_only: - return None # superseded/culled while we waited out of lock — don't clobber the newer one - old = _sessions.pop(reg, None) - if old is not None: - _kill(old) - shutil.rmtree(directory, ignore_errors=True) - directory.mkdir(parents=True, exist_ok=True) - proc = subprocess.Popen( - _ffmpeg_cmd( - src, - start_s, + doomed: list[tuple[HlsSession, Path | None]] = [] + # Finished even when the new session fails to start (no space, no ffmpeg, a DB error): what + # was moved aside under the lock is otherwise left for a restart's sweep. + try: + with _lock: + if replace_only is not None and _sessions.get(reg) is not replace_only: + return None # superseded/culled while we waited out of lock — don't clobber the newer one + old = _sessions.pop(reg, None) + if old is not None: + doomed.append(_detach(old)) + # An ffmpeg still exiting from THIS path (the old session, or one stopped a moment ago) + # would write into the directory about to be made; see `_dying`. + prev = _dying.pop(directory, None) + if prev is not None: + _await_exit(key, prev) + shutil.rmtree(directory, ignore_errors=True) + directory.mkdir(parents=True, exist_ok=True) + proc = subprocess.Popen( + _ffmpeg_cmd( + src, + start_s, + directory, + audio_ord, + aoff, + audio_langs if is_multi else None, + transcode=transcode, + hw=hw, + tonemap=tonemap, + preset=preset, + crf=crf, + max_height=max_height, + loudnorm=loudnorm, + ), + stdout=subprocess.DEVNULL, + stderr=subprocess.DEVNULL, + preexec_fn=preexec, + ) + s = HlsSession( + key, directory, - audio_ord, - aoff, - audio_langs if is_multi else None, + proc, + start_s, + variants, transcode=transcode, hw=hw, tonemap=tonemap, - preset=preset, - crf=crf, - max_height=max_height, - loudnorm=loudnorm, - ), - stdout=subprocess.DEVNULL, - stderr=subprocess.DEVNULL, - preexec_fn=preexec, - ) - s = HlsSession( - key, - directory, - proc, - start_s, - variants, - transcode=transcode, - hw=hw, - tonemap=tonemap, - user_id=user_id, - ) - _sessions[reg] = s - # The one line that says which pipeline a session actually got. Everything downstream - # — the cap, the HW→SW fallback, the subtitle offset — is about this choice, and until - # now it was only inferable from the ffmpeg command nobody keeps. - log.info( - "an HLS session started", - extra=logs.event( - "plex.stream.session_started", - key=key, - transcode=transcode, - hw=hw, - tonemap=tonemap, - variants=len(variants or ()), - start_s=round(start_s, 1), - ), - ) - # Admin-configurable caps (Configuration → Plex); at least 1 each so playback can't be - # capped to zero. - _enforce_caps( - user_id, - max(1, sysconfig.get_int(db, "plex_max_transcodes")), - max(1, sysconfig.get_int(db, "plex_max_sessions")), - ) + user_id=user_id, + ) + _sessions[reg] = s + # The one line that says which pipeline a session actually got. Everything downstream + # — the cap, the HW→SW fallback, the subtitle offset — is about this choice, and until + # now it was only inferable from the ffmpeg command nobody keeps. + log.info( + "an HLS session started", + extra=logs.event( + "plex.stream.session_started", + key=key, + transcode=transcode, + hw=hw, + tonemap=tonemap, + variants=len(variants or ()), + start_s=round(start_s, 1), + ), + ) + # Admin-configurable caps (Configuration → Plex); at least 1 each so playback can't be + # capped to zero. + doomed += _enforce_caps( + user_id, + max(1, sysconfig.get_int(db, "plex_max_transcodes")), + max(1, sysconfig.get_int(db, "plex_max_sessions")), + ) + finally: + _finish(doomed) return s s = _spawn(*_select_encode(db, item, transcode, src)) @@ -974,6 +1045,11 @@ def artifact_may_arrive(s: "HlsSession", filename: str) -> bool: # session, absent for a single-audio one. if variant not in (s.variants or (None,)): return False + # And the name must be the one ffmpeg SPELLS for that sequence: `int()` also reads `seg_007.ts` + # and a non-ASCII digit run as a sequence the session may have reached, and neither file is + # ever written — a yes there parked the request for the whole wait. + if filename != segment_name(seq, variant): + return False if seq == 0: return True try: @@ -1108,16 +1184,16 @@ def reap_idle(finished_idle_s: float = _FINISHED_IDLE_S) -> int: `reap_abandoned` bought the same safety by keeping every finished session for ten minutes — each one a whole segmented copy of the film on a volume shared with the download center.""" now = time.time() - dropped = 0 + doomed: list[tuple[HlsSession, Path | None]] = [] with _lock: for reg, s in list(_sessions.items()): done = s.proc.poll() is not None idle = now - s.last_access if idle > _SESSION_IDLE_S or (done and idle > finished_idle_s): - _kill(s) _sessions.pop(reg, None) - dropped += 1 - return dropped + doomed.append(_detach(s)) + _finish(doomed) + return len(doomed) def stop_session(user_id: int, key: str, session_id: str) -> bool: @@ -1132,8 +1208,9 @@ def stop_session(user_id: int, key: str, session_id: str) -> bool: s = _sessions.get((user_id, key)) if s is None or s.id != session_id: return False - _kill(s) _sessions.pop((user_id, key), None) + doomed = [_detach(s)] + _finish(doomed) return True diff --git a/backend/app/routes/plex/playback.py b/backend/app/routes/plex/playback.py index 77b7c231..043dc26a 100644 --- a/backend/app/routes/plex/playback.py +++ b/backend/app/routes/plex/playback.py @@ -234,6 +234,15 @@ def person_image(u: str, request: Request) -> Response: return imgcache.response(data, ct) +def _supersede_resume_pushes(user_id: int, item_id: int, now: datetime) -> None: + """A watched mark supersedes every resume push still in flight for the item: stamping it as the + latest push makes `_resume_push_superseded` drop them. Popping the entry instead (to keep the map + small) removed the very guard, and an older resume push could then land at Plex AFTER the + scrobble and revive a resume point on a watched item. Eviction bounds the map.""" + with _push_throttle_lock: + _last_push_at[(user_id, item_id)] = now + + def _push_resume_guarded( user_id: int, item_id: int, @@ -329,12 +338,7 @@ def item_progress( near_end = duration > 0 and position > duration - _FINISH_MARGIN_S if near_end: flipped = service.mark_finished(db, user.id, it.id, now) - # The finish supersedes every resume push still in flight for this item: stamping it as the - # latest push makes `_resume_push_superseded` drop them. Popping the entry instead (to keep - # the map small) removed the very guard, and an older resume push could then land at Plex - # AFTER the scrobble and revive a resume point on a watched item. Eviction bounds the map. - with _push_throttle_lock: - _last_push_at[(user.id, it.id)] = now + _supersede_resume_pushes(user.id, it.id, now) db.commit() # Only the checkpoint that flipped the row scrobbles: a later checkpoint of the same finished # item — or a second one racing this one — would otherwise bump Plex's viewCount again. @@ -352,7 +356,11 @@ def item_progress( push_now = _should_push_resume(user.id, it.id, now, final) # Marked dirty before the (background) push so the reconcile knows to re-push if it fails, and so # `push_state_to_plex` can settle synced_to_plex once Plex confirms. - service.checkpoint(db, user, it.id, st, position, now, dirty=push_now) + if not service.checkpoint(db, user, it.id, st, position, now, dirty=push_now): + # Marked watched a moment ago: this is the finishing player's late tick (see + # `service.checkpoint`), and the answer is the same one a near-end checkpoint gets. + db.commit() + return {"ok": True, "status": "watched", "position_seconds": 0} db.commit() if push_now: _push_watch( @@ -408,6 +416,8 @@ def item_state( _push_watch(background, db, user.id, it.id, rating_key, "unwatched") return {"ok": True, "status": "new"} service.set_status(db, user, it.id, st, status) + if status == "watched": + _supersede_resume_pushes(user.id, it.id, datetime.now(timezone.utc)) db.commit() if status == "watched" and prev_status != "watched": _push_watch(background, db, user.id, it.id, rating_key, "watched") @@ -427,6 +437,10 @@ def _bulk_state( always reads the committed marks.""" pushable = plex_watch.link_for_push(db, user_id) is not None to_push = service.bulk_state(db, user_id, eps, watched) + if watched: + now = datetime.now(timezone.utc) + for item_id, _rating_key, _action in to_push: + _supersede_resume_pushes(user_id, item_id, now) if pushable and to_push: # One coalesced background task for the whole batch (reuses a single session + Plex client), # not one task per episode — a 200-ep show mark is 1 task, not 200. @@ -632,9 +646,10 @@ def stream_hls_file( try: content = path.read_bytes() except OSError: - # The session was torn down between the wait and the read — `_kill` rmtrees the whole - # directory, and nothing holds `_lock` across those two steps. It has to present as the same - # 404 a gone session does: the player's recovery is keyed on the STATUS (`code === 404`), so + # The session was torn down between the wait and the read — teardown moves the whole + # directory aside and deletes it, and nothing holds `_lock` across those two steps. It has + # to present as the same 404 a gone session does: the player's recovery is keyed on the + # STATUS (`code === 404`), so # a 500 here skipped the reclaim and ended playback with "couldn't play" instead of # restarting at the playhead. Ordinary trigger: `loadSession` awaits the new session — which # kills and rmtrees the old one — before destroying the old hls instance, so the outgoing diff --git a/backend/tests/test_hls_reaper.py b/backend/tests/test_hls_reaper.py index c7dbac3e..844f5499 100644 --- a/backend/tests/test_hls_reaper.py +++ b/backend/tests/test_hls_reaper.py @@ -83,6 +83,7 @@ class _Proc: class _Session: def __init__(self, tmp_path, name: str, finished: bool, idle_s: float): + self.key = name self.dir = tmp_path / name self.dir.mkdir() self.proc = _Proc(finished) diff --git a/backend/tests/test_hls_teardown.py b/backend/tests/test_hls_teardown.py new file mode 100644 index 00000000..58fe2aa6 --- /dev/null +++ b/backend/tests/test_hls_teardown.py @@ -0,0 +1,158 @@ +"""Tearing a session down does not stall everybody else's playback. + +Every segment request (`current_session`) and every checkpoint (`touch_session`) takes the one +registry `_lock`. Stopping a session waits up to 3 s for its ffmpeg to exit and then deletes a whole +segmented film, ~1200 files — and that used to happen under the same lock, so one viewer's seek, a +cap cull or a reap pass held up every other viewer's HLS requests for seconds, each one a parked +threadpool worker and its pooled DB connection. + +What still has to be atomic is pinned too: a session directory is reused for the same item, offset +and tag, so a new session on that path must not start before the old ffmpeg — which writes by path +— has exited. +""" + +import threading +import time + +import pytest + +from app.plex import stream + + +class _SlowProc: + """An ffmpeg that takes a while to exit after TERM: `wait` blocks until `exited` is set.""" + + def __init__(self, order: list[str], name: str): + self.order = order + self.name = name + self.waiting = threading.Event() + self.exited = threading.Event() + + def poll(self): + return 0 if self.exited.is_set() else None + + def terminate(self): + pass + + def kill(self): + self.exited.set() + + def wait(self, timeout=None): + self.waiting.set() + if not self.exited.wait(timeout): + raise stream.subprocess.TimeoutExpired("ffmpeg", timeout) + self.order.append(f"{self.name} exited") + return 0 + + +class _Session: + def __init__(self, directory, proc, key: str): + self.id = f"sid-{key}" + self.key = key + self.dir = directory + self.proc = proc + self.last_access = time.time() + self.transcode = False + + +@pytest.fixture +def sessions(monkeypatch): + live: dict = {} + monkeypatch.setattr(stream, "_sessions", live) + monkeypatch.setattr(stream, "_dying", {}, raising=False) + return live + + +def test_stopping_a_session_does_not_hold_up_another_viewers_requests(tmp_path, sessions): + order: list[str] = [] + slow = _SlowProc(order, "a") + d = tmp_path / "1_u1_0_0_+0.00" + d.mkdir() + (d / "seg_0.ts").write_bytes(b"x") + sessions[(1, "a")] = _Session(d, slow, "a") + sessions[(2, "b")] = _Session(tmp_path / "2_u2_0_0_+0.00", _SlowProc(order, "b"), "b") + + stopper = threading.Thread(target=stream.stop_session, args=(1, "a", "sid-a")) + stopper.start() + try: + assert slow.waiting.wait(2.0), "the stop never waited for its ffmpeg" + got: list = [] + reader = threading.Thread(target=lambda: got.append(stream.current_session(2, "b"))) + reader.start() + reader.join(1.0) + assert not reader.is_alive(), "a segment request waited on another viewer's teardown" + assert got and got[0] is sessions[(2, "b")] + finally: + slow.exited.set() + stopper.join(5.0) + # The teardown still finishes: the session is gone and nothing of its directory is left behind. + assert (1, "a") not in sessions + assert sorted(p.name for p in tmp_path.iterdir()) == [] + + +def test_a_reap_pass_does_not_hold_up_another_viewers_requests(tmp_path, sessions): + order: list[str] = [] + slow = _SlowProc(order, "a") + d = tmp_path / "1_u1_0_0_+0.00" + d.mkdir() + gone = _Session(d, slow, "a") + gone.last_access = time.time() - stream._SESSION_IDLE_S - 5 + sessions[(1, "a")] = gone + sessions[(2, "b")] = _Session(tmp_path / "2_u2_0_0_+0.00", _SlowProc(order, "b"), "b") + + reaper = threading.Thread(target=stream.reap_idle) + reaper.start() + try: + assert slow.waiting.wait(2.0), "the reap never waited for its ffmpeg" + toucher = threading.Thread(target=stream.touch_session, args=(2, "b")) + toucher.start() + toucher.join(1.0) + assert not toucher.is_alive(), "a checkpoint waited on another viewer's teardown" + finally: + slow.exited.set() + reaper.join(5.0) + assert list(sessions) == [(2, "b")] + assert not d.exists() + + +class _FakeItem: + rating_key = "12345" + file_path = "/media/movie.mkv" + playable = "remux" + codec_video = "h264" + + +def test_a_new_session_on_a_dying_ones_path_starts_only_after_it_exited( + tmp_path, sessions, monkeypatch +): + """The same item at the same second gets the same directory. The old ffmpeg writes by PATH, so + while it is still winding down after TERM it can write a segment — or, on a clean exit, a final + playlist with ENDLIST — into the NEW session's directory.""" + order: list[str] = [] + procs = iter([_SlowProc(order, "old"), _SlowProc(order, "new")]) + + def popen(*a, **k): + order.append("popen") + return next(procs) + + monkeypatch.setattr(stream, "_HLS_ROOT", tmp_path) + src = tmp_path / "movie.mkv" + src.write_bytes(b"x") + monkeypatch.setattr(stream.paths, "local_media_path", lambda db, fp: src) + monkeypatch.setattr(stream.sysconfig, "get_int", lambda db, k: 2) + monkeypatch.setattr(stream.sysconfig, "get_bool", lambda db, k: False) + monkeypatch.setattr(stream.subprocess, "Popen", popen) + monkeypatch.setattr(stream, "wait_for", lambda *a, **k: True) + + first = stream.start_session(db=None, user_id=7, item=_FakeItem(), start_s=0.0) + assert first is not None + stopper = threading.Thread(target=stream.stop_session, args=(7, "12345", first.id)) + stopper.start() + assert first.proc.waiting.wait(2.0) + threading.Timer(0.3, first.proc.exited.set).start() + second = stream.start_session(db=None, user_id=7, item=_FakeItem(), start_s=0.0) + stopper.join(5.0) + assert second is not None and second.dir == first.dir + assert order.index("old exited") < order.index("popen", 1) + assert second.dir.is_dir(), "the old teardown deleted the new session's directory" + second.proc.exited.set() diff --git a/backend/tests/test_plex_finish.py b/backend/tests/test_plex_finish.py index fea93d65..c4e1d1d2 100644 --- a/backend/tests/test_plex_finish.py +++ b/backend/tests/test_plex_finish.py @@ -13,7 +13,7 @@ from datetime import datetime, timedelta, timezone from sqlalchemy import select from app.db import SessionLocal -from app.plex.models import PlexItem, PlexLibrary, PlexState +from app.plex.models import PlexItem, PlexLibrary, PlexSeason, PlexShow, PlexState from app.plex import service as plex_service from app.routes.plex import playback @@ -81,3 +81,150 @@ def test_the_finish_supersedes_a_resume_push_still_in_flight(db, as_user, make_u ) assert r.json() == {"ok": True, "status": "watched", "position_seconds": 0} assert playback._resume_push_superseded(user.id, item.id, earlier) is True + + +def test_a_checkpoint_sent_before_the_finish_does_not_land_on_the_watched_row( + db, as_user, make_user, monkeypatch +): + """The 10 s tick posted at 2699 of 3000 s (not near the end) races the credits auto-skip's + "watched". When the mark commits first, the late checkpoint used to write its position onto the + watched row and schedule a resume push after the scrobble: Plex showed the item in progress, and + a Plex-authoritative pull imported it back.""" + user = make_user() + item = _item(db) + monkeypatch.setattr(playback.plex_stream, "touch_session", lambda uid, key: None) + client = as_user(user) + assert client.post("/api/plex/item/m1/state", json={"status": "watched"}).status_code == 200 + + r = client.post( + "/api/plex/item/m1/progress", json={"position_seconds": 2699, "duration_seconds": 3000} + ) + assert r.status_code == 200, r.text + assert r.json() == {"ok": True, "status": "watched", "position_seconds": 0} + db.expire_all() + st = db.execute(select(PlexState).where(PlexState.item_id == item.id)).scalars().one() + assert (st.status, st.position_seconds) == ("watched", 0) + + +def test_a_rewatch_records_its_position_once_the_finish_is_behind_it( + db, as_user, make_user, monkeypatch +): + """The other half: a watched item played again is still checkpointed — only a position arriving + right on the heels of the mark is read as that mark's late tick.""" + user = make_user() + item = _item(db) + long_ago = datetime.now(timezone.utc) - timedelta(days=3) + db.add(PlexState(user_id=user.id, item_id=item.id, status="watched", watched_at=long_ago)) + db.commit() + monkeypatch.setattr(playback.plex_stream, "touch_session", lambda uid, key: None) + r = as_user(user).post( + "/api/plex/item/m1/progress", json={"position_seconds": 600, "duration_seconds": 3000} + ) + assert r.json() == {"ok": True, "position_seconds": 600} + db.expire_all() + st = db.execute(select(PlexState).where(PlexState.item_id == item.id)).scalars().one() + assert (st.status, st.position_seconds) == ("watched", 600) + + +def test_marking_watched_supersedes_a_resume_push_still_in_flight( + db, as_user, make_user, monkeypatch +): + """The same guard the near-end finish has: a resume push scheduled before the mark must not reach + Plex after its scrobble.""" + user = make_user() + item = _item(db) + earlier = datetime.now(timezone.utc) - timedelta(seconds=5) + assert playback._should_push_resume(user.id, item.id, earlier, final=True) + r = as_user(user).post("/api/plex/item/m1/state", json={"status": "watched"}) + assert r.status_code == 200, r.text + assert playback._resume_push_superseded(user.id, item.id, earlier) is True + + +def _state(db, user, item): + db.expire_all() + st = ( + db.execute( + select(PlexState).where(PlexState.user_id == user.id, PlexState.item_id == item.id) + ) + .scalars() + .one() + ) + return st.status, st.position_seconds + + +def test_a_mark_committed_after_the_checkpoints_read_still_refuses_it(db, make_user): + """The race itself, not its sequential shadow: the route read the row while it was still in + progress, and the finish committed from another session before the checkpoint wrote. Only a write + conditional on the row as it is NOW refuses that position.""" + user = make_user() + item = _item(db) + db.add(PlexState(user_id=user.id, item_id=item.id, status="new", position_seconds=2690)) + db.commit() + now = datetime.now(timezone.utc) + read = plex_service.state_row(db, user, item.id) # what the checkpoint's route read: "new" + assert read.status == "new" + + other = SessionLocal() + try: + assert plex_service.mark_finished(other, user.id, item.id, now) is True + other.commit() + finally: + other.close() + + assert plex_service.checkpoint(db, user, item.id, read, 2699, now, dirty=True) is False + db.commit() + assert _state(db, user, item) == ("watched", 0) + + +def test_a_tick_seconds_behind_the_mark_is_refused_a_rewatch_a_minute_later_is_not(db, make_user): + """A position right behind the mark is that mark's late tick; one a minute later is a rewatch. + Literal times on purpose: a window widened to an hour must fail here, not move with it.""" + user = make_user() + item = _item(db) + now = datetime.now(timezone.utc) + st = PlexState(user_id=user.id, item_id=item.id, status="watched", watched_at=now) + db.add(st) + db.commit() + + st.watched_at = now - timedelta(seconds=20) + db.commit() + assert plex_service.checkpoint(db, user, item.id, st, 600, now, dirty=True) is False + db.commit() + st.watched_at = now - timedelta(seconds=60) + db.commit() + assert plex_service.checkpoint(db, user, item.id, st, 600, now, dirty=True) is True + db.commit() + assert _state(db, user, item) == ("watched", 600) + + +def test_marking_a_season_watched_supersedes_its_episodes_resume_pushes(db, as_user, make_user): + """The bulk mark has the same guard as one item's: an episode's resume push scheduled before + the season was marked must not reach Plex after the scrobble.""" + user = make_user() + lib = PlexLibrary(plex_key="2", title="Shows", kind="show", enabled=True) + db.add(lib) + db.flush() + sh = PlexShow(rating_key="sh1", library_id=lib.id, title="Show") + db.add(sh) + db.flush() + se = PlexSeason(rating_key="se1", show_id=sh.id, season_number=1) + db.add(se) + db.flush() + ep = PlexItem( + rating_key="e1", + library_id=lib.id, + kind="episode", + title="E1", + show_id=sh.id, + season_id=se.id, + season_number=1, + episode_number=1, + ) + db.add(ep) + db.commit() + earlier = datetime.now(timezone.utc) - timedelta(seconds=5) + assert playback._should_push_resume(user.id, ep.id, earlier, final=True) + + r = as_user(user).post("/api/plex/season/se1/state", json={"watched": True}) + assert r.json() == {"ok": True, "changed": 1, "watched": True} + assert playback._resume_push_superseded(user.id, ep.id, earlier) is True diff --git a/backend/tests/test_plex_hls_file_route.py b/backend/tests/test_plex_hls_file_route.py index 56571696..bf58d4b4 100644 --- a/backend/tests/test_plex_hls_file_route.py +++ b/backend/tests/test_plex_hls_file_route.py @@ -198,6 +198,31 @@ def test_a_name_the_session_cannot_produce_fails_at_once(client, tmp_path, sessi assert elapsed < 2.0, f"waited {elapsed:.1f}s for a playlist this session never writes" +@pytest.mark.parametrize( + ("name", "audio_langs"), + [ + ("seg_00.ts", None), # `int()` reads it as segment 0, which needs no predecessor + ("seg_007.ts", None), # …as segment 7, whose predecessor seg_6.ts is on disk + ("seg_٧.ts", None), # an Arabic-Indic 7: `\d` and `int()` both take it + ("seg_a0_07.ts", ["hu", "en"]), + ("seg_0_00.ts", ["hu", "en"]), + ], +) +def test_a_segment_name_ffmpeg_never_spells_is_refused(tmp_path, name, audio_langs): + """ffmpeg writes `seg_%d.ts`: no padding, ASCII digits. A name that only PARSES to a sequence the + session has reached is still a file that will never appear, and saying yes to it parked the + request for the full 20 s wait — the parking this guard exists to end.""" + d = tmp_path / "m1_0_0_+0.00" + d.mkdir() + for v in plex_stream.session_variants(audio_langs) or (None,): + (d / plex_stream.segment_name(6, v)).write_bytes(b"x") + sess = _Session("sid-a", d, running=True, audio_langs=audio_langs) + assert not plex_stream.artifact_may_arrive(sess, name) + # The canonical spelling of the same next segment is still accepted. + v = (plex_stream.session_variants(audio_langs) or (None,))[0] + assert plex_stream.artifact_may_arrive(sess, plex_stream.segment_name(7, v)) + + @pytest.mark.parametrize("digits", [100, 5000]) def test_an_absurd_segment_index_is_a_404_not_a_500(client, tmp_path, sessions, digits): """`int()` on an unbounded digit run raises ValueError past 4300 digits, and only OSError was @@ -213,7 +238,7 @@ def test_an_absurd_segment_index_is_a_404_not_a_500(client, tmp_path, sessions, def test_a_session_torn_down_during_the_read_is_a_404_not_a_500( client, monkeypatch, tmp_path, sessions ): - """`_kill` rmtrees the whole directory and nothing holds `_lock` across the wait and the read, + """Teardown moves the whole directory aside and nothing holds `_lock` across the wait and the read, so the file can vanish in between — on every seek-restart, since `loadSession` awaits the new session before destroying the old hls instance. The player's recovery is keyed on the 404 STATUS, so a 500 here skipped the reclaim and ended playback with "couldn't play".""" diff --git a/backend/tests/test_plex_service.py b/backend/tests/test_plex_service.py index 3858db86..181e0cc3 100644 --- a/backend/tests/test_plex_service.py +++ b/backend/tests/test_plex_service.py @@ -348,7 +348,8 @@ def test_an_early_checkpoint_drops_a_bare_state_and_zeroes_a_marked_one(db, make user = make_user() lib = _lib(db) bare, hid = _movie(db, lib, "bare"), _movie(db, lib, "hid") - st = service.checkpoint(db, user, bare.id, None, 120, NOW, dirty=True) + assert service.checkpoint(db, user, bare.id, None, 120, NOW, dirty=True) + st = queries.state(db, user, bare.id) assert (st.position_seconds, st.synced_to_plex) == (120, False) _state(db, user, hid, status="hidden", position_seconds=40) service.clear_progress(db, queries.state(db, user, bare.id), NOW) @@ -429,7 +430,8 @@ def test_a_first_write_that_lost_the_race_updates_the_winners_row(db, make_user) user = make_user() it = _movie(db, _lib(db), "m-race") _state(db, user, it, position_seconds=10) # the request that won, committed - st = service.checkpoint(db, user, it.id, None, 42, NOW, dirty=True) # read "none" before it + assert service.checkpoint(db, user, it.id, None, 42, NOW, dirty=True) # read "none" before it + st = queries.state(db, user, it.id) assert (st.position_seconds, st.synced_to_plex) == (42, False) st = service.set_status(db, user, it.id, None, "hidden") assert st.status == "hidden" diff --git a/backend/tests/test_plex_session_stop.py b/backend/tests/test_plex_session_stop.py index b375be7d..7f6c2c06 100644 --- a/backend/tests/test_plex_session_stop.py +++ b/backend/tests/test_plex_session_stop.py @@ -39,6 +39,7 @@ class _Proc: class _Session: def __init__(self, session_id: str, directory): self.id = session_id + self.key = "m1" self.dir = directory self.proc = _Proc() self.last_access = time.time() diff --git a/backend/tests/test_plex_stream.py b/backend/tests/test_plex_stream.py index baffdf28..379d02e1 100644 --- a/backend/tests/test_plex_stream.py +++ b/backend/tests/test_plex_stream.py @@ -26,6 +26,9 @@ from app.plex.stream import _SESSION_DIR_RE "12345_0_2_+0.00", # current shape, explicit audio_ord "12345_842_multi_-1.25", # multi-audio, negative offset "12345_842_multi_+12.50", + # a torn-down session's directory, moved aside to be deleted outside the lock — left behind + # only if the process died between the rename and the delete + "12345_u7_842_multi_+12.50.dead-0123abcd", ], ) def test_our_session_directories_are_swept(name): @@ -47,6 +50,9 @@ def test_our_session_directories_are_swept(name): "snapshot_2024_backup_+1.00", # session-ish tail, but the key isn't a rating_key "backup_2024_multi_+0.00", ".config", + "12345_u7_0_0_+0.00.dead-", # a tombstone suffix with no id + "12345_u7_0_0_+0.00.dead-notours!", + "backup_u7_0_0_+0.00.dead-0123abcd", ], ) def test_foreign_directories_are_left_alone(name): @@ -159,7 +165,7 @@ class TestSweepOrphans: class _FakeProc: - """Stand-in for the ffmpeg Popen — start_session only stores it (and _kill/reap poll it later).""" + """Stand-in for the ffmpeg Popen — start_session only stores it (and teardown/reap poll it later).""" def poll(self): return None @@ -1052,3 +1058,63 @@ class TestSessionsPerViewer: self._start(monkeypatch, user_id=uid) assert len(stream._sessions) == 3 and (1, "12345") not in stream._sessions assert oldest.proc.killed and not oldest.transcode + + +class TestTeardownFinishes: + """A session taken down by a cap or replaced by its own viewer is only renamed aside under the + lock; it is deleted after. The rename alone already moves the directory and `terminate()` alone + already marks the proc killed — so what is pinned here is the tomb: a whole segmented copy of the + film that only a restart's sweep would otherwise reclaim.""" + + capturing_env = TestTranscodeStartSession.capturing_env + + @staticmethod + def _left_behind(root): + return sorted(p.name for p in root.iterdir() if stream._TOMB in p.name) + sorted( + str(p) for p in stream._dying + ) + + def test_a_culled_session_is_deleted_not_only_moved_aside(self, capturing_env, monkeypatch): + monkeypatch.setattr(stream, "_dying", {}) + first = TestSessionsPerViewer._start(monkeypatch, user_id=1, rating_key="1") + first.last_access -= 60 + TestSessionsPerViewer._start(monkeypatch, user_id=1, rating_key="2") + TestSessionsPerViewer._start(monkeypatch, user_id=1, rating_key="3") + assert first.proc.killed and not first.dir.exists() + assert self._left_behind(capturing_env) == [] + + def test_a_viewers_restart_deletes_the_session_it_replaces(self, capturing_env, monkeypatch): + monkeypatch.setattr(stream, "_dying", {}) + monkeypatch.setattr( + stream, "_select_encode", lambda db, item, transcode, src: (False, False) + ) + monkeypatch.setattr( + stream, "_probe_first_pts", lambda seg: None + ) # no real segment to probe + old = stream.start_session(db=None, user_id=1, item=_FakeItem(), start_s=0.0) + new = stream.start_session(db=None, user_id=1, item=_FakeItem(), start_s=600.0) + assert old.proc.killed and new.dir.exists() and not old.dir.exists() + assert self._left_behind(capturing_env) == [] + + def test_a_restart_that_fails_to_start_still_deletes_what_it_replaced( + self, capturing_env, monkeypatch + ): + # The new ffmpeg cannot start (a full disk, no ffmpeg, no memory): the old session is already + # moved aside, and nothing else in this process would ever delete it. + monkeypatch.setattr(stream, "_dying", {}) + monkeypatch.setattr( + stream, "_select_encode", lambda db, item, transcode, src: (False, False) + ) + monkeypatch.setattr( + stream, "_probe_first_pts", lambda seg: None + ) # no real segment to probe + old = stream.start_session(db=None, user_id=1, item=_FakeItem(), start_s=0.0) + + def no_ffmpeg(*args, **kwargs): + raise OSError(28, "No space left on device") + + monkeypatch.setattr(stream.subprocess, "Popen", no_ffmpeg) + with pytest.raises(OSError): + stream.start_session(db=None, user_id=1, item=_FakeItem(), start_s=600.0) + assert old.proc.killed and not old.dir.exists() + assert self._left_behind(capturing_env) == [] diff --git a/docs/DESIGN.md b/docs/DESIGN.md index e81a0579..01c946fa 100644 --- a/docs/DESIGN.md +++ b/docs/DESIGN.md @@ -378,7 +378,11 @@ Which actions are visible and which live in a menu. managing the shares, re-download, the copies, cancel / delete / remove behind their confirms); a link inside a row keeps the browser's own right-click menu. A notification row keeps its link and its unread mark, shows its dismiss on hover (in the "…" on touch), and its - "…" holds find / undo for an activity notice. The notification center's band keeps Mark all read + "…" holds find / undo for an activity notice. The notification center lists what asks for the + reader first ("Needs you · n": an unread row, a notice waiting on an action — one read during the + visit keeps its place there until Mark all read), then one stream of both sources, newest first, + and folds read rows older than a week under "Earlier (n)", a heading-style disclosure, closed — Back to a row + that folded away since opens it on that row. Its band keeps Mark all read and puts Clear everything, now behind a confirm, in its "…"; the Scheduler's strip keeps Run all and Pause and puts Purge discovery in its "…". A list whose row objects are rebuilt each render or replaced by a poll passes `useContextMenu({ keyOf })` (its id), so the "…" still closes its own menu. A menu's @@ -824,3 +828,4 @@ of a full-width panel and nothing on the right. | 2026-10-25 | Player panel glyphs (owner 1A, 2A, 3) | In the side panel the queue row is glyph-only on one line, and Mark watched is a glyph toggle among the actions (✓ / ✓✓); the docked bar keeps its labels; the related switch at the default size, "Trending" | Labelled buttons wrapping onto three lines; glyphs on the docked bar too; Mark watched on its own line | The panel is narrow and the list controls are the same few every time; each glyph keeps its full name and shortcut in its tooltip; the docked bar has the width for words | | 2026-10-25 | Auto-advance glyphs (owner E A, H a) | Auto-advance wears list glyphs (`ListEnd` next and off, `ListStart` previous, `Shuffle` random), in the panel and the docked bar; the auto-advance and loop cycles are `lit`, not `pressed` | The skip arrows for the mode; an X'd list for off; `aria-pressed` on a cycle | Beside Previous / Next the skip arrows read as a second Next; a cycle of three or four modes is no two-state switch, and its name already says which | | 2026-10-25 | Restart and Reset glyphs (owner L A) | A menu's Play from the beginning wears `RotateCcw`, as the card's own Restart does; Reset (clear progress and status) wears `Eraser`; Redownload wears `RefreshCw`, the retry arrow | The skip arrow ⏮ on the menu's Restart; `RotateCcw` on both Restart and Reset; `TimerReset` for Restart | ⏮ is Previous beside the player; one action wore two glyphs, and two actions in one menu would have worn one | +| 2026-10-28 | Notification order (owner A-D1 a, A-D2 a, ★ a) | What asks for the reader first, from either source ("Needs you · n"), then one stream newest first; read rows older than a week fold under "Earlier (n)", closed, its label in the section-heading style with a chevron | One stream with no band; this browser's activity above the server's (the two fixed sections); a server-side retention that deletes old rows | An admin's access requests sat under up to a hundred server rows; week-old read notices crowded what is new; folding keeps the rows (nothing is deleted) and an unread row never folds, whatever its age | diff --git a/frontend/src/components/ui/Modal.dom.test.tsx b/frontend/src/components/ui/Modal.dom.test.tsx index 6e22ab35..4169ed69 100644 --- a/frontend/src/components/ui/Modal.dom.test.tsx +++ b/frontend/src/components/ui/Modal.dom.test.tsx @@ -23,7 +23,7 @@ vi.mock("../../lib/history", () => ({ useBackToClose: () => {} })); import Modal from "./Modal"; import { clearAll, notify } from "../../lib/notifications"; import { pushLayer, removeLayer, useLayer } from "../../lib/layerStack"; -import { flushSync } from "react-dom"; +import { createPortal, flushSync } from "react-dom"; // Fire a keydown on window (where Modal installs its handler), inside act. const press = (key: string, shiftKey = false) => @@ -311,6 +311,100 @@ describe("Modal focus trap (offsetParent stubbed 'visible')", () => { document.body.removeChild(trigger); }); + it("hands its trigger to the dialog left standing above it, which restores it on close", () => { + // The standing upper dialog captured ITS restore target while the focus was inside the lower + // one — a control that is gone once the lower closes. So the lower skipping its own restore + // (above) handed the trigger to no one, and closing the upper left the focus on . + // The arrangement keyboard help or the error dialog opened over a dialog actually has. + const trigger = document.createElement("button"); + document.body.appendChild(trigger); + trigger.focus(); + function Siblings() { + const [lower, setLower] = useState(true); + const [upper, setUpper] = useState(false); + return ( + <> + {lower && ( + {}}> + + + + )} + {upper && ( + {}}> + + + )} + + ); + } + render(); + screen.getByText("open upper").focus(); + act(() => void screen.getByText("open upper").click()); // opened from inside the lower dialog + const inUpper = screen.getByText("close upper"); + inUpper.focus(); + // The lower closes on its own under the standing upper (`.click()` moves no focus). + act(() => void screen.getByText("close lower").click()); + expect(screen.queryByText("close lower")).toBeNull(); + expect(document.activeElement).toBe(inUpper); + act(() => void inUpper.click()); + expect(document.activeElement).toBe(trigger); + document.body.removeChild(trigger); + }); + + it("restores the trigger when it closes with the focus in one of its menus", () => { + // The popover engine portals a menu out of the dialog's root (it carries no aria-modal), so a + // focus on a menu row read as "the user walked away": Back with a menu open inside a dialog + // left the focus on . The portal marker is the one `ui/AnchoredPopover` reads too. + const trigger = document.createElement("button"); + document.body.appendChild(trigger); + trigger.focus(); + const portal = document.createElement("div"); + portal.setAttribute("data-floating-ui-portal", ""); + document.body.appendChild(portal); + const { unmount } = render( + {}}> + + {createPortal(, portal)} + , + ); + screen.getByText("menu-row").focus(); + unmount(); + expect(document.activeElement).toBe(trigger); + document.body.removeChild(trigger); + document.body.removeChild(portal); + }); + + it("does not pull the focus out of a menu that stays open when it closes on its own", () => { + // The other side of counting a menu row as held: a menu that is NOT this dialog's — a page's + // "…" left open while a dialog finishes in the background — stays standing, and a dialog that + // closes on its own (a write's success) must not take the focus from it to its trigger. + const trigger = document.createElement("button"); + document.body.appendChild(trigger); + trigger.focus(); + const portal = document.createElement("div"); + portal.setAttribute("data-floating-ui-portal", ""); + const row = document.createElement("button"); + row.textContent = "page-menu-row"; + portal.appendChild(row); + document.body.appendChild(portal); + function SelfClosing() { + const [open, setOpen] = useState(true); + return open ? ( + {}}> + + + ) : null; + } + render(); + row.focus(); + act(() => void screen.getByText("finish").click()); // `.click()` moves no focus + expect(screen.queryByText("finish")).toBeNull(); + expect(document.activeElement).toBe(row); + document.body.removeChild(trigger); + document.body.removeChild(portal); + }); + it("leaves the focus alone when the user moved it out of the dialog themselves", () => { // The other half of the same rule, and the branch nothing drove: taking focus back from where // the user deliberately put it is the rudest thing a closing dialog can do. diff --git a/frontend/src/components/ui/PageScroller.dom.test.tsx b/frontend/src/components/ui/PageScroller.dom.test.tsx index 04317e5b..a4c199a2 100644 --- a/frontend/src/components/ui/PageScroller.dom.test.tsx +++ b/frontend/src/components/ui/PageScroller.dom.test.tsx @@ -17,6 +17,7 @@ vi.mock("react-i18next", async (importOriginal) => { import PageScroller from "./PageScroller"; import BackToTop from "./BackToTop"; import { notifyNav } from "../../lib/history"; +import { revealedNavItem } from "../../lib/listNav"; beforeAll(() => { // useScrollFade wires a ResizeObserver (watch-the-children); jsdom ships none. @@ -125,6 +126,38 @@ describe("PageScroller scroll memory", () => { expect(main.scrollTop).toBe(450); }); + // Back focuses an item the restore did not expect — one that moved to another list while the page + // was away — and scrolls it into view: the restore's next frame put the old offset back over it. + it("gives the page up to an item brought into view while it restores", () => { + let frames: FrameRequestCallback[] = []; + vi.stubGlobal("requestAnimationFrame", (cb: FrameRequestCallback) => frames.push(cb)); + const flush = (n: number) => { + for (let i = 0; i < n; i++) { + const run = frames; + frames = []; + for (const cb of run) cb(0); + } + }; + setEntry(1); + const { container } = render( + +
+ , + ); + const main = sizedMain(container); + main.scrollTop = 450; + fireEvent.scroll(main); + setEntry(2); + notifyNav(); + flush(1); + setEntry(1); + notifyNav(); // Back: the restore to 450 waits for its first frame + main.scrollTop = 120; // the item scrolled into view + revealedNavItem(container.querySelector("#moved")!); + flush(5); + expect(main.scrollTop).toBe(120); + }); + // Landing at the end shows Back to top, whose spacer then brings the spot within reach. it("finishes the way to a spot the spacer brings within reach after it lands", () => { let frames: FrameRequestCallback[] = []; diff --git a/frontend/src/components/ui/PageScroller.tsx b/frontend/src/components/ui/PageScroller.tsx index 66eb92c0..1f2fc90d 100644 --- a/frontend/src/components/ui/PageScroller.tsx +++ b/frontend/src/components/ui/PageScroller.tsx @@ -12,6 +12,7 @@ import { StickyTopProvider } from "./stickyTop"; import { useBackToTopShown, useCornerLift } from "./backToTopState"; import { CONTENT_ID } from "./landmarks"; import { ensureNavId, peekNavId, subscribeNav } from "../../lib/history"; +import { NAV_REVEAL_EVENT } from "../../lib/listNav"; // Saved scroll offsets, keyed by `${historyEntryId}:${scrollKey}`. The entry id ties an offset to the // exact history entry, so Back restores where you left it and a forward navigation (a new entry) opens @@ -63,11 +64,15 @@ function restoreScroll(el: HTMLElement, top: number): () => void { el.removeEventListener("wheel", stop); el.removeEventListener("touchstart", stop); el.removeEventListener("pointerdown", stop); + el.removeEventListener(NAV_REVEAL_EVENT, stop); document.removeEventListener("keydown", stop, true); }; el.addEventListener("wheel", stop, { passive: true }); el.addEventListener("touchstart", stop, { passive: true }); el.addEventListener("pointerdown", stop); + // So does an item the keyboard brought into view (`lib/listNav`): Back focusing one that is not + // where the restored offset shows. + el.addEventListener(NAV_REVEAL_EVENT, stop); document.addEventListener("keydown", stop, true); const tick = () => { if (cancelled) return; diff --git a/frontend/src/components/ui/dialogFocus.ts b/frontend/src/components/ui/dialogFocus.ts index c757b169..35a00c20 100644 --- a/frontend/src/components/ui/dialogFocus.ts +++ b/frontend/src/components/ui/dialogFocus.ts @@ -75,6 +75,16 @@ export function lockBodyScroll(): () => void { }; } +// A popover engine surface (`ui/AnchoredPopover`): portalled out of the dialog's root and carrying no +// aria-modal, so a focus on one of a dialog's menu rows is inside the dialog all the same. +const FLOATING = "[data-floating-ui-portal]"; + +// Restore targets handed to a dialog left standing above one that closed (see the cleanup below): +// that dialog's own target was captured while the focus was in the one that closed, and is gone. +// Keyed by the standing dialog's aria-modal element; a WeakMap, so a dialog that never closes +// through this hook leaves nothing behind. +const handedRestore = new WeakMap(); + /** The rest of the contract: lock the page behind, open focused, hand the focus back on close. * `scopeRef` is the panel the focus should land in; `ownerRef`, when the dialog is bigger than its * panel, is that whole dialog — it must be the SAME element `trapTab` is scoped to, or the two @@ -128,6 +138,11 @@ export function useDialogChrome( // Today that reads the same either way (a detached owner and a null one both fail every // `contains` below), but the cleanup should not depend on a ref React has already let go of. const ownerAtOpen = ownerRef?.current ?? scope; + // This dialog's aria-modal element, the key a closing dialog under it hands its target by. + const modalAtOpen = + ownerAtOpen?.closest('[aria-modal="true"]') ?? + ownerAtOpen?.querySelector('[aria-modal="true"]') ?? + null; if (scope && !scope.contains(document.activeElement)) { const back = focusedRef.current; const target = @@ -144,10 +159,12 @@ export function useDialogChrome( // it — and while it held the focus we read that as the user walking away. Then both closing // together left the focus on : ours skipped the restore, and the child's own // `restoreTo` was already detached (passive cleanups run parent-first). Measured in jsdom, - // and it is the ordinary case — an error dialog raised over a dialog. + // and it is the ordinary case — an error dialog raised over a dialog. And one of its own + // menus (see FLOATING): Back with a menu open closed the dialog with the focus on a menu row. heldRef.current = (owner?.contains(active) ?? false) || - (active instanceof Element && active.closest('[aria-modal="true"]') != null); + (active instanceof Element && + (active.closest('[aria-modal="true"]') != null || active.closest(FLOATING) != null)); // Where to put the focus back on a re-mount, though: the PANEL's own last focus. A toast is // gone by then, so restoring onto it would strand the focus exactly as before. if (scopeRef.current?.contains(document.activeElement)) @@ -172,16 +189,28 @@ export function useDialogChrome( // this, a dialog closing on its own (an in-flight mutation's `onSuccess`) under a modal raised // above it pulled the focus out of that standing dialog onto our own trigger, behind its // scrim — where Enter still activated it. + // + // Skipped, the trigger is handed to that standing dialog instead: ITS target was captured + // while the focus was in ours, so it is gone now, and it falls back to the one handed to it. + // A popover still standing (a menu of a dialog that stays open) is not ours to take from + // either, though there is no dialog to hand the trigger to. const owner = ownerAtOpen; const active = document.activeElement; + const target = restoreTo?.isConnected + ? restoreTo + : modalAtOpen + ? handedRestore.get(modalAtOpen) + : undefined; const above = active instanceof Element ? active.closest('[aria-modal="true"]') : null; const standingAbove = above !== null && above.isConnected && !above.contains(owner as Node | null) && !(owner?.contains(above) ?? false); - if (heldRef.current && !standingAbove && restoreTo && restoreTo.isConnected) - restoreTo.focus(); + const standingMenu = active instanceof Element && active.closest(FLOATING)?.isConnected; + if (!heldRef.current || !target?.isConnected) return; + if (standingAbove) handedRestore.set(above, target); + else if (!standingMenu) target.focus(); }; }, [restoreTo, scopeRef, ownerRef, focusScope]); } diff --git a/frontend/src/components/ui/pressableSelectable.contract.test.ts b/frontend/src/components/ui/pressableSelectable.contract.test.ts index 5425ae26..e764232e 100644 --- a/frontend/src/components/ui/pressableSelectable.contract.test.ts +++ b/frontend/src/components/ui/pressableSelectable.contract.test.ts @@ -38,6 +38,10 @@ const PLAIN: Record = { "shell/NavSidebar.tsx": [3, "the account trigger and its menu rows"], "features/header/PaletteLauncher.tsx": [2, "launcher controls"], "modules/playlists/PlaylistsRail.tsx": [1, "the load-error retry"], + "modules/notifications/NotificationsPanel.tsx": [ + 1, + "the Earlier (n) fold's heading (a disclosure: its text is its label)", + ], "modules/plex/PlexCollectionEditor.tsx": [1, "a checkbox row in a picker"], "modules/plex/PlexInfo.tsx": [1, "the rating filter (a number)"], "modules/plex/PlexPlayerSettings.tsx": [1, "the clock's date-style options"], diff --git a/frontend/src/content/release-notes/en.json b/frontend/src/content/release-notes/en.json index a691894b..1020ac4f 100644 --- a/frontend/src/content/release-notes/en.json +++ b/frontend/src/content/release-notes/en.json @@ -1,4 +1,15 @@ [ + { + "version": "0.114.0", + "date": "2026-10-29", + "new": [ + "Notifications: what asks for you comes first, then everything else as one stream, newest first. Read notices older than a week fold under Earlier, and Back returns to the row you left." + ], + "fixed": [ + "Plex: a progress save sent just before an episode ends no longer leaves it unwatched, and a refused finish no longer sticks.", + "After a dialog closes, the keyboard focus returns to where it was, also inside a menu that stays open." + ] + }, { "version": "0.113.0", "date": "2026-10-28", diff --git a/frontend/src/content/release-notes/hu.json b/frontend/src/content/release-notes/hu.json index 7ae3364c..654764f4 100644 --- a/frontend/src/content/release-notes/hu.json +++ b/frontend/src/content/release-notes/hu.json @@ -1,4 +1,15 @@ [ + { + "version": "0.114.0", + "date": "2026-10-29", + "new": [ + "Értesítések: elöl az, ami rád vár, utána minden más egy folyamban, a legújabb elöl. Az egy hétnél régebbi olvasott értesítések a Korábbiak alá csukódnak, és a Vissza arra a sorra tér vissza, ahonnan elmentél." + ], + "fixed": [ + "Plex: az epizód vége előtt közvetlenül elküldött mentés már nem hagyja megnézetlenül, és egy elutasított befejezés sem ragad be.", + "Párbeszédablak bezárása után a billentyűfókusz oda tér vissza, ahol volt, egy nyitva maradó menüben is." + ] + }, { "version": "0.113.0", "date": "2026-10-28", diff --git a/frontend/src/i18n/locales/en/inbox.json b/frontend/src/i18n/locales/en/inbox.json index a34194aa..76680a88 100644 --- a/frontend/src/i18n/locales/en/inbox.json +++ b/frontend/src/i18n/locales/en/inbox.json @@ -6,11 +6,15 @@ "clearConfirmTitle": "Clear every notification?", "clearConfirmBody": "Every notification here leaves the list, read or not. They don't come back.", "markRead": "Mark read", + "unreadName": "{{name}}, unread", "dismiss": "Dismiss", "rowMenu": "Notification actions", "menu": "Notifications actions", - "sectionSystem": "System", - "sectionActivity": "Activity", + "sectionNeedsYou_one": "Needs you · {{count}}", + "sectionNeedsYou_other": "Needs you · {{count}}", + "sectionRecent": "Recent", + "earlier_one": "Earlier ({{count}})", + "earlier_other": "Earlier ({{count}})", "andMore_one": "and {{count}} more", "andMore_other": "and {{count}} more", "openScheduler": "Open Scheduler", diff --git a/frontend/src/i18n/locales/hu/inbox.json b/frontend/src/i18n/locales/hu/inbox.json index 5ad64a7e..a50c51fc 100644 --- a/frontend/src/i18n/locales/hu/inbox.json +++ b/frontend/src/i18n/locales/hu/inbox.json @@ -6,11 +6,15 @@ "clearConfirmTitle": "Törlöd az összes értesítést?", "clearConfirmBody": "Minden itteni értesítés eltűnik a listából, olvasott és olvasatlan is. Nem hozhatók vissza.", "markRead": "Olvasottnak jelöl", + "unreadName": "{{name}}, olvasatlan", "dismiss": "Elrejtés", "rowMenu": "Értesítés műveletei", "menu": "Értesítések műveletei", - "sectionSystem": "Rendszer", - "sectionActivity": "Tevékenység", + "sectionNeedsYou_one": "Rád vár · {{count}}", + "sectionNeedsYou_other": "Rád vár · {{count}}", + "sectionRecent": "Legutóbbiak", + "earlier_one": "Korábbiak ({{count}})", + "earlier_other": "Korábbiak ({{count}})", "andMore_one": "és még {{count}}", "andMore_other": "és még {{count}}", "openScheduler": "Ütemező megnyitása", diff --git a/frontend/src/lib/inboxOrder.test.ts b/frontend/src/lib/inboxOrder.test.ts new file mode 100644 index 00000000..a4e0357f --- /dev/null +++ b/frontend/src/lib/inboxOrder.test.ts @@ -0,0 +1,113 @@ +import { describe, expect, it } from "vitest"; +import { FOLD_AFTER_MS, entryKey, orderInbox, type InboxOrder } from "./inboxOrder"; +import type { AppNotification } from "./api"; +import type { Notification as ClientNotif } from "./notifications"; + +const NOW = Date.parse("2026-10-28T12:00:00Z"); +const ago = (ms: number) => NOW - ms; +const H = 3600 * 1000; +const D = 24 * H; + +const server = (id: number, at: number | null, read: boolean) => + ({ + id, + type: "download_done", + title: "t", + body: null, + data: null, + read, + dismissed: false, + created_at: at === null ? null : new Date(at).toISOString(), + }) as AppNotification; +const client = (id: number, at: number, extra: Partial = {}) => + ({ + id, + level: "info", + message: "m", + requiresInteraction: false, + ts: at, + read: true, + dismissed: true, + transient: false, + repeat: 1, + ...extra, + }) as ClientNotif; + +const keys = (o: InboxOrder) => ({ + needsYou: o.needsYou.map(entryKey), + recent: o.recent.map(entryKey), + earlier: o.earlier.map(entryKey), +}); +const order = (s: AppNotification[], c: ClientNotif[], held: ReadonlySet = new Set()) => + keys(orderInbox(s, c, { now: NOW, held })); + +describe("orderInbox", () => { + it("puts a waiting access-requests nudge above every server row, read or not", () => { + const o = order( + [server(1, ago(1 * H), false), server(2, ago(1 * D), true)], + [client(1, ago(5 * 60 * 1000), { meta: { kind: "access-requests" } })], + ); + expect(o.needsYou).toEqual(["client:1", "server:1"]); + expect(o.recent).toEqual(["server:2"]); + }); + + it("an old unread row still asks: it never folds or drifts below newer read ones", () => { + const o = order([server(1, ago(30 * D), false), server(2, ago(1 * H), true)], []); + expect(o.needsYou).toEqual(["server:1"]); + expect(o.recent).toEqual(["server:2"]); + expect(o.earlier).toEqual([]); + }); + + it("merges both sources into one stream, newest first", () => { + const o = order( + [server(1, ago(3 * H), true), server(2, ago(1 * H), true)], + [client(7, ago(2 * H)), client(8, ago(4 * H))], + ); + expect(o.recent).toEqual(["server:2", "client:7", "server:1", "client:8"]); + }); + + it("folds read rows older than a week, and only those", () => { + const o = order( + [server(1, ago(FOLD_AFTER_MS + H), true), server(2, ago(FOLD_AFTER_MS - H), true)], + [client(3, ago(9 * D))], + ); + expect(o.recent).toEqual(["server:2"]); + expect(o.earlier).toEqual(["server:1", "client:3"]); + }); + + it("keeps a row that asked earlier in the visit in its place once read, from either source", () => { + // Opening the center marks the client notices read: the band must not empty as it renders; and + // a server row marked read must not jump lists under the reader. + const o = order( + [server(1, ago(30 * D), true), server(2, ago(3 * H), true)], + [client(4, ago(2 * H)), client(5, ago(1 * H))], + new Set(["client:4", "server:1"]), + ); + expect(o.needsYou).toEqual(["client:4", "server:1"]); + expect(o.recent).toEqual(["client:5", "server:2"]); + expect(o.earlier).toEqual([]); + }); + + it("an unread client notice (raised while the center is open) asks", () => { + const o = order([], [client(6, ago(H), { read: false })]); + expect(o.needsYou).toEqual(["client:6"]); + }); + + it("a notice waiting on an interaction asks until it is dealt with", () => { + const o = order( + [], + [ + client(1, ago(H), { requiresInteraction: true, dismissed: false }), + client(2, ago(H), { requiresInteraction: true, dismissed: true }), + ], + ); + expect(o.needsYou).toEqual(["client:1"]); + expect(o.recent).toEqual(["client:2"]); + }); + + it("a server row of no known date sorts last and never folds", () => { + const o = order([server(1, null, true), server(2, ago(30 * D), true)], [client(3, ago(H))]); + expect(o.recent).toEqual(["client:3", "server:1"]); + expect(o.earlier).toEqual(["server:2"]); + }); +}); diff --git a/frontend/src/lib/inboxOrder.ts b/frontend/src/lib/inboxOrder.ts new file mode 100644 index 00000000..4fdfb65d --- /dev/null +++ b/frontend/src/lib/inboxOrder.ts @@ -0,0 +1,72 @@ +import type { AppNotification } from "./api"; +import type { Notification as ClientNotif } from "./notifications"; + +// The notification center's order, in ONE rule for both sources. It used to be two fixed sections, +// the server's above this browser's — so an admin's "access requests waiting" sat under up to a +// hundred server rows, week-old read "Download ready" notices among them. Now what asks for the +// reader comes first, then everything else as one time-ordered stream, and read rows older than a +// week fold away (kept, not deleted). + +export type InboxEntry = + | { src: "server"; n: AppNotification; at: number } + | { src: "client"; n: ClientNotif; at: number }; + +export interface InboxOrder { + /** Unread, or waiting on an action: newest first. Never folded, whatever its age. */ + needsYou: InboxEntry[]; + /** The rest from the last week (or of no known date), newest first. */ + recent: InboxEntry[]; + /** Read rows older than `FOLD_AFTER_MS`, newest first — shown only on request. */ + earlier: InboxEntry[]; +} + +export const FOLD_AFTER_MS = 7 * 24 * 3600 * 1000; + +// A row of no known date (a server row stored without one) sorts last and never folds: its age is +// unknown, and hiding it on a guess is how a row goes missing. +const NO_DATE = Number.NEGATIVE_INFINITY; + +function serverAt(n: AppNotification): number { + const at = n.created_at ? Date.parse(n.created_at) : NaN; + return Number.isNaN(at) ? NO_DATE : at; +} + +/** A row's key across both sources: their ids are separate sequences, and they share one list now. */ +export const keyOf = (src: InboxEntry["src"], id: number): string => `${src}:${id}`; +export const entryKey = (e: InboxEntry): string => keyOf(e.src, e.n.id); + +/** Whether a client notice asks for the reader by what it is: an access-requests nudge for as long + * as it exists (its producer removes it when nothing is pending), a notice that waits on an + * interaction until it is dealt with. */ +export const clientAsks = (n: ClientNotif): boolean => + n.meta?.kind === "access-requests" || (n.requiresInteraction && !n.dismissed); + +export function orderInbox( + server: readonly AppNotification[], + client: readonly ClientNotif[], + /** `held`: the rows that asked earlier in this visit (entry keys). They keep their place once read — + * a row marked read must not jump to another list under the reader, taking the keyboard focus to + * as it remounts there — and they are how a client notice stays up at all: opening the + * center marks every client notice read, so "unread" alone would empty the band as it renders. */ + { now, held }: { now: number; held: ReadonlySet }, +): InboxOrder { + const out: InboxOrder = { needsYou: [], recent: [], earlier: [] }; + const place = (e: InboxEntry, asks: boolean) => { + if (asks) out.needsYou.push(e); + else if (e.at !== NO_DATE && now - e.at > FOLD_AFTER_MS) out.earlier.push(e); + else out.recent.push(e); + }; + for (const n of server) + place({ src: "server", n, at: serverAt(n) }, !n.read || held.has(keyOf("server", n.id))); + for (const n of client) + place( + { src: "client", n, at: n.ts }, + !n.read || held.has(keyOf("client", n.id)) || clientAsks(n), + ); + // Newest first; equal times keep their source's own order (Array.sort is stable). + const newest = (a: InboxEntry, b: InboxEntry) => (b.at === a.at ? 0 : b.at > a.at ? 1 : -1); + out.needsYou.sort(newest); + out.recent.sort(newest); + out.earlier.sort(newest); + return out; +} diff --git a/frontend/src/lib/listNav.restoreMoved.dom.test.tsx b/frontend/src/lib/listNav.restoreMoved.dom.test.tsx new file mode 100644 index 00000000..9bf46723 --- /dev/null +++ b/frontend/src/lib/listNav.restoreMoved.dom.test.tsx @@ -0,0 +1,206 @@ +// @vitest-environment jsdom +// +// Back to a page whose item changed lists while it was away. The notification center holds a row +// that asked for the reader in "Needs you" for the visit; on the next visit the same row, read now, +// sits in the stream below. The restore matched the unnamed list by its place, missed the name there +// and fell back to the saved index — the focus landed on the neighbour, and Enter opened the wrong +// notification. Its own file: listNav keeps module-level state. +import { afterEach, describe, expect, it, vi } from "vitest"; +import { act, cleanup, render } from "@testing-library/react"; +import { useRef } from "react"; + +import { + NAV_REVEAL_EVENT, + navItemAttrs, + navPosition, + pendingNavName, + restoreNavPosition, + useListNav, +} from "./listNav"; + +afterEach(cleanup); + +// `count`: a virtualized list's, which counts rows it has yet to render. +function List({ items, count }: { items: string[]; count?: number }) { + const ref = useRef(null); + useListNav(ref, { enabled: true, count }); + return ( +
+ {items.map((t, i) => ( +
+ +
+ ))} +
+ ); +} + +// `tray`: a list of the app's own, outside the page (the running downloads), whose rows can share a +// name with the page's. +const Page = ({ + lists, + tray, + counts = [], +}: { + lists: string[][]; + tray?: string[]; + counts?: number[]; +}) => ( + <> +
+ {lists.map((items, i) => ( + + ))} +
+ {tray && } + +); + +const button = (name: string) => + [...document.querySelectorAll("button")].find((b) => b.textContent === name)!; +const focused = () => document.activeElement?.closest("[data-nav-index]")?.textContent; +const wait = (ms: number) => act(() => new Promise((r) => setTimeout(r, ms))); + +async function leaveAndReturn( + before: string[][], + from: string, + after: string[][], + tray?: string[], +) { + const { unmount } = render(); + button(from).focus(); + const pos = navPosition()!; + unmount(); // the page switch + render(); // Back + const cancel = restoreNavPosition(pos); + await wait(120); + cancel(); +} + +describe("Back to an item that moved to another list", () => { + it("finds it by its name in the list it moved to, not its neighbour at the old place", async () => { + await leaveAndReturn([["subscribed", "download"], ["older"]], "subscribed", [ + ["download"], + ["subscribed", "older"], + ]); + expect(focused()).toBe("subscribed"); + }); + + it("finds it when the list it left is gone altogether", async () => { + await leaveAndReturn([["subscribed"], ["older"]], "subscribed", [["subscribed", "older"]]); + expect(focused()).toBe("subscribed"); + }); + + it("does not guess between two items of that name elsewhere: the saved place", async () => { + await leaveAndReturn([["twin", "download"], ["older"]], "twin", [ + ["download"], + ["twin", "older", "twin"], + ]); + expect(focused()).toBe("download"); + }); + + it("brings an item found in another list into view; one in its own list stays put", async () => { + // The scroll restore put the page where the item WAS: found elsewhere, it would hold the focus + // off screen. + const reveal = vi.fn(); + Element.prototype.scrollIntoView = reveal; + // ...and tells the page's scroll restore, which would otherwise put the old offset back over it. + const told = vi.fn(); + document.addEventListener(NAV_REVEAL_EVENT, told); + await leaveAndReturn([["subscribed", "download"], ["older"]], "subscribed", [ + ["download"], + ["subscribed", "older"], + ]); + expect(reveal).toHaveBeenCalledTimes(1); + expect(told).toHaveBeenCalledTimes(1); + cleanup(); + reveal.mockClear(); + told.mockClear(); + await leaveAndReturn([["subscribed", "download"]], "download", [["download", "subscribed"]]); + expect(focused()).toBe("download"); + expect(reveal).not.toHaveBeenCalled(); + expect(told).not.toHaveBeenCalled(); + document.removeEventListener(NAV_REVEAL_EVENT, told); + }); + + it("never takes it from a list outside the page: the download tray's row of that name", async () => { + // A feed card and its download in the tray are both named by the video's title. + await leaveAndReturn( + [["subscribed", "download"]], + "subscribed", + [["download"]], + ["subscribed"], + ); + expect(focused()).toBe("download"); + }); + + it("waits for its own list to render the place before looking in another one", async () => { + // A virtualized list puts its rows back a frame after the page: the item is still to come at + // the first look, and the one of that name in the rail below is not it. + const { unmount } = render(); + button("subscribed").focus(); + const pos = navPosition()!; + unmount(); + const { rerender } = render(); + const cancel = restoreNavPosition(pos); + await wait(60); + rerender(); + await wait(120); + cancel(); + expect(document.activeElement?.closest("main > div")).toBe( + document.querySelector("main")!.firstElementChild, + ); + }); + + it("looks in another list at its last look when its own never renders the place it counts", async () => { + vi.useFakeTimers(); + try { + const { unmount } = render(); + button("subscribed").focus(); + const pos = navPosition()!; + unmount(); + // Its own list still counts two, and keeps showing one; the item is in the next list. + render(); + const cancel = restoreNavPosition(pos); + await act(async () => vi.advanceTimersByTimeAsync(1000)); + expect(focused()).toBeUndefined(); // still giving its own list the chance + await act(async () => vi.advanceTimersByTimeAsync(1100)); + expect(focused()).toBe("subscribed"); + cancel(); + } finally { + vi.useRealTimers(); + } + }); +}); + +describe("the name a Back is still looking for", () => { + const pos = { list: 0, lists: 1, index: 0, name: "subscribed" }; + + it("is there while it looks, and gone once cancelled", () => { + const cancel = restoreNavPosition(pos); + expect(pendingNavName()).toBe("subscribed"); + cancel(); + expect(pendingNavName()).toBeNull(); + }); + + it("is gone once the focus went somewhere else", async () => { + render(); + const cancel = restoreNavPosition(pos); + button("elsewhere").focus(); + await wait(80); + expect(pendingNavName()).toBeNull(); + cancel(); + }); + + it("is gone once it gave up", async () => { + vi.useFakeTimers(); + try { + const cancel = restoreNavPosition(pos); + await vi.advanceTimersByTimeAsync(2100); + expect(pendingNavName()).toBeNull(); + cancel(); + } finally { + vi.useRealTimers(); + } + }); +}); diff --git a/frontend/src/lib/listNav.ts b/frontend/src/lib/listNav.ts index 6d688fea..5b3e5df8 100644 --- a/frontend/src/lib/listNav.ts +++ b/frontend/src/lib/listNav.ts @@ -42,6 +42,15 @@ const lists = new Map(); // The item we gave a tabindex to, so the next move can take it back — and whether we named it too. let roved: HTMLElement | null = null; let named = false; +// A named item's label follows its spoken state while it holds the focus: Mark read hands the +// focus to the row BEFORE the row reads read, and the label given then said "unread" until the +// keyboard moved on. +const relabel = + typeof MutationObserver === "undefined" + ? null + : new MutationObserver(() => { + if (named && roved) roved.setAttribute("aria-label", spokenName(roved)); + }); const INDEX = "data-nav-index"; const LIST = "data-nav-list"; @@ -56,14 +65,24 @@ const REVEAL_FRAMES = 10; * `inset` draws the keyboard ring INSIDE the item. The ring sits 2 px outside by default, which a * card with a gap round it has room for; an item flush against its neighbours (a log line) or against * the edge of a clipping card (a Plex playlist show header) does not: the ring ran past the list, or - * was clipped away entirely. A table row needs no flag: index.css insets every `tr` by its tag. */ + * was clipped away entirely. A table row needs no flag: index.css insets every `tr` by its tag. + * + * `label` is what is SPOKEN instead of `name`, when it carries a state the item can lose while the + * page is away (a notification's "unread"): `name` is also how Back finds the item again, and a + * name that changed with the state would miss it. */ export const navItemAttrs = ( index: number, name?: string | null, - { inset = false }: { inset?: boolean } = {}, -): { "data-nav-index": number; "data-nav-name"?: string; "data-nav-inset"?: "" } => ({ + { inset = false, label }: { inset?: boolean; label?: string } = {}, +): { + "data-nav-index": number; + "data-nav-name"?: string; + "data-nav-label"?: string; + "data-nav-inset"?: ""; +} => ({ [INDEX]: index, ...(name ? { "data-nav-name": name } : null), + ...(label ? { "data-nav-label": label } : null), ...(inset ? { "data-nav-inset": "" } : null), }); @@ -204,11 +223,23 @@ function itemName(el: HTMLElement): string { ).trim(); } +/** What a screen reader hears while the item holds the focus: its `label` when it gives one. */ +const spokenName = (el: HTMLElement): string => el.getAttribute("data-nav-label") || itemName(el); + +/** The event an item brought into view sends up its page: a scroll restore still under way gives the + * page up to it (`ui/PageScroller`) — on Back, an item found in another list than the one it was + * left in is not where the restored offset shows, and the restore's next frame scrolled it away. */ +export const NAV_REVEAL_EVENT = "navreveal"; +export const revealedNavItem = (el: HTMLElement): void => { + el.dispatchEvent(new Event(NAV_REVEAL_EVENT, { bubbles: true })); +}; + function focusItem(el: HTMLElement, reveal = true): void { if (roved !== el) { if (named) roved?.removeAttribute("role"); if (named) roved?.removeAttribute("aria-label"); roved?.removeAttribute("tabindex"); + relabel?.disconnect(); roved = null; named = false; // An item that is a Tab stop in its own right (a native