From e96a562f14cefc9dbf381bb645c83dfa41869413 Mon Sep 17 00:00:00 2001 From: npeter83 Date: Wed, 28 Oct 2026 18:48:21 +0100 Subject: [PATCH 01/22] fix(plex): a segment name ffmpeg never spells is refused; teardown no longer holds the registry lock The artifact guard parsed the sequence with int(), so a zero-padded or non-ASCII-digit name read as a segment the session had reached and parked the sync route for the full 20 s wait. It now also requires the canonical spelling segment_name() produces. Stopping, replacing, culling or reaping a session waited up to 3 s for ffmpeg and deleted a whole segmented film under _lock, which every segment request and checkpoint takes. Teardown now signals the process and moves its directory aside under the lock, and waits/deletes after it. A new session on the same path still waits for the dying ffmpeg first (it writes by path), and the orphan sweep recognises a moved-aside directory left by a crash. --- backend/app/plex/stream.py | 111 ++++++++++++--- backend/app/routes/plex/playback.py | 7 +- backend/tests/test_hls_reaper.py | 1 + backend/tests/test_hls_teardown.py | 158 ++++++++++++++++++++++ backend/tests/test_plex_hls_file_route.py | 27 +++- backend/tests/test_plex_session_stop.py | 1 + backend/tests/test_plex_stream.py | 8 +- 7 files changed, 289 insertions(+), 24 deletions(-) create mode 100644 backend/tests/test_hls_teardown.py diff --git a/backend/app/plex/stream.py b/backend/app/plex/stream.py index f273710d..0f957a03 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,12 +819,18 @@ 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 + doomed: list[tuple[HlsSession, Path | 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) + 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( @@ -816,11 +882,12 @@ def start_session( ) # Admin-configurable caps (Configuration → Plex); at least 1 each so playback can't be # capped to zero. - _enforce_caps( + doomed += _enforce_caps( user_id, max(1, sysconfig.get_int(db, "plex_max_transcodes")), max(1, sysconfig.get_int(db, "plex_max_sessions")), ) + _finish(doomed) return s s = _spawn(*_select_encode(db, item, transcode, src)) @@ -974,6 +1041,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 +1180,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 +1204,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..e946d25d 100644 --- a/backend/app/routes/plex/playback.py +++ b/backend/app/routes/plex/playback.py @@ -632,9 +632,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_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_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..9d56e8ca 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 From 24190e4c16376e8d0bb81da486b5fd231603a762 Mon Sep 17 00:00:00 2001 From: npeter83 Date: Wed, 28 Oct 2026 18:56:12 +0100 Subject: [PATCH 02/22] fix(plex): a checkpoint sent just before the finish no longer lands on the watched row The 10 s tick in flight when the player finishes (or a refusal rollback whose state write in fact committed) wrote its mid-item position onto the freshly watched row and scheduled a resume push after the scrobble, so Plex showed the item in progress again. The checkpoint is now a conditional write that skips a row marked watched in the last 30 s and answers like a finish; a rewatch past that is checkpointed as before. Marking an item (or a season/show) watched also supersedes resume pushes still in flight, as the near-end finish already did. --- backend/app/plex/service.py | 39 +++++++++++++++----- backend/app/routes/plex/playback.py | 28 ++++++++++---- backend/tests/test_plex_finish.py | 57 +++++++++++++++++++++++++++++ backend/tests/test_plex_service.py | 6 ++- 4 files changed, 112 insertions(+), 18 deletions(-) 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/routes/plex/playback.py b/backend/app/routes/plex/playback.py index e946d25d..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. diff --git a/backend/tests/test_plex_finish.py b/backend/tests/test_plex_finish.py index fea93d65..f264ba57 100644 --- a/backend/tests/test_plex_finish.py +++ b/backend/tests/test_plex_finish.py @@ -81,3 +81,60 @@ 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 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" From 7e5d4a7d930e8e8d5153c18d6ae1ec8b28b84631 Mon Sep 17 00:00:00 2001 From: npeter83 Date: Wed, 28 Oct 2026 19:05:24 +0100 Subject: [PATCH 03/22] fix(ui): a closing dialog's focus restore reaches the dialog left standing above it, and survives a menu A dialog closing on its own under a standing modal skipped its restore, and the standing modal's own target was a control inside the closed one: closing it left the focus on . The skipped target is now handed to the standing dialog, which falls back to it. Focus on a row of one of the dialog's own menus (the popover engine portals them out of the dialog root) read as the user walking away, so Back with a menu open left the focus on . A focus in a popover portal now counts as held, and one still standing is not taken from. --- frontend/src/components/ui/Modal.dom.test.tsx | 66 ++++++++++++++++++- frontend/src/components/ui/dialogFocus.ts | 37 +++++++++-- 2 files changed, 98 insertions(+), 5 deletions(-) diff --git a/frontend/src/components/ui/Modal.dom.test.tsx b/frontend/src/components/ui/Modal.dom.test.tsx index 6e22ab35..85facf5c 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,70 @@ 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("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/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]); } From 0d410592e2372ce1bf41d90ef2b6bc86d7053dfb Mon Sep 17 00:00:00 2001 From: npeter83 Date: Wed, 28 Oct 2026 19:05:24 +0100 Subject: [PATCH 04/22] fix(plex): a stale finish refusal no longer sticks, and a refusal keeps the player's detail A refusal that landed after a replay raised the refusal flag for the finish that watch made later, so leaving skipped the watched patch. Each finish now resets the flag and a refusal acts only for the finish it answers. A refusal while the player still sat on the item (the up-next card) removed the detail query it observes, dropping the card and restarting its countdown, or losing it if the refetch failed. It is now refetched under the player and dropped only when the player leaves. --- .../lib/plexPlayerHooks.finish.dom.test.tsx | 119 ++++++++++++++++++ frontend/src/lib/plexPlayerHooks.ts | 36 ++++-- 2 files changed, 147 insertions(+), 8 deletions(-) create mode 100644 frontend/src/lib/plexPlayerHooks.finish.dom.test.tsx diff --git a/frontend/src/lib/plexPlayerHooks.finish.dom.test.tsx b/frontend/src/lib/plexPlayerHooks.finish.dom.test.tsx new file mode 100644 index 00000000..42b50e60 --- /dev/null +++ b/frontend/src/lib/plexPlayerHooks.finish.dom.test.tsx @@ -0,0 +1,119 @@ +// @vitest-environment jsdom +// +// The finish latch's refusal path, against a real query cache. +// +// A refusal answers ONE finish: a replay begun while it was in flight starts a new watch, and a +// refusal that lands after it must not take back the finish that watch later makes — that left the +// cache saying unwatched, mid-episode, for an item the server had watched. +// +// And a refusal while the player is still on the item (the up-next card, the default end state) must +// not empty the detail that player observes: the card is gated on it, so it dropped, came back with its +// countdown restarted when the refetch landed, and never came back if that refetch failed too. The +// entry is dropped when the player leaves, where the next open has to fetch the truth. +import { describe, it, expect, vi, afterEach, beforeEach } from "vitest"; +import { act, cleanup, renderHook } from "@testing-library/react"; +import { QueryClient, QueryClientProvider, useQuery } from "@tanstack/react-query"; +import { type ReactNode } from "react"; +import { usePlexProgress } from "./plexPlayerHooks"; +import { api, type PlexItemDetail } from "./api"; +import { qk } from "./queryKeys"; +import i18n, { plexLang } from "../i18n"; + +const RK = "4242"; +const key = () => qk.plexItem(RK, plexLang(i18n.language)); +const detail = { id: RK, status: "new", position_seconds: 600 } as unknown as PlexItemDetail; + +function deferred() { + let reject: (e: unknown) => void = () => {}; + const promise = new Promise((_, rej) => { + reject = rej; + }); + return { promise, reject }; +} + +const flush = () => + act(async () => { + await Promise.resolve(); + await Promise.resolve(); + }); + +describe("usePlexProgress — a refused finish", () => { + let qc: QueryClient; + let video: HTMLVideoElement; + beforeEach(() => { + qc = new QueryClient({ defaultOptions: { queries: { retry: false } } }); + video = document.createElement("video"); + vi.spyOn(api, "plexProgress").mockResolvedValue({ ok: true } as never); + }); + afterEach(() => { + cleanup(); + vi.restoreAllMocks(); + }); + + const setup = (observe: boolean) => { + const wrapper = ({ children }: { children: ReactNode }) => ( + {children} + ); + // Stable, as the player's own refs are: the effect re-runs on a new ref object, and a re-run + // starts the item over. + const refs = { + videoRef: { current: video }, + absRef: { current: 1500 }, + durationRef: { current: 3000 }, + }; + return renderHook( + () => { + useQuery({ + queryKey: key(), + queryFn: () => Promise.resolve(detail), + enabled: observe, + }); + return usePlexProgress({ + id: RK, + qc, + ...refs, + }); + }, + { wrapper }, + ); + }; + + it.each([ + ["before", true], + ["after", false], + ])( + "a refusal landing after a replay, %s its next finish, takes back neither", + async (_, early) => { + qc.setQueryData(key(), detail); + const { result } = setup(false); + const first = deferred(); + act(() => result.current.finish(first.promise)); + act(() => void video.dispatchEvent(new Event("play"))); // the viewer plays it again + if (early) { + first.reject(new Error("refused")); // only now is the first finish refused + await flush(); + } + act(() => result.current.finish(Promise.resolve())); + await flush(); + if (!early) { + first.reject(new Error("refused")); + await flush(); + } + act(() => result.current.settle()); + expect(qc.getQueryData(key())).toMatchObject({ status: "watched", position_seconds: 0 }); + }, + ); + + it("keeps the detail the still-open player observes, and drops it when the player leaves", async () => { + const { result, unmount } = setup(true); + await flush(); + expect(qc.getQueryData(key())).toBeDefined(); + const refused = deferred(); + act(() => result.current.finish(refused.promise)); + refused.reject(new Error("refused")); + await flush(); + expect(qc.getQueryData(key())).toBeDefined(); + unmount(); + expect(qc.getQueryData(key())).toBeUndefined(); + }); +}); diff --git a/frontend/src/lib/plexPlayerHooks.ts b/frontend/src/lib/plexPlayerHooks.ts index dbc20408..13a5a166 100644 --- a/frontend/src/lib/plexPlayerHooks.ts +++ b/frontend/src/lib/plexPlayerHooks.ts @@ -214,6 +214,14 @@ export function usePlexProgress(opts: { // write; this closes the other three. What the refusal does take back is the // CLAIM: the cache must not say watched, and `settle` must not re-assert it on the way out. let finishRefused = false; + // Whether the player has left the item (`settle` ran): what a refusal does to the cached detail + // depends on whether the player is still observing it. + let left = false; + // Which finish a refusal answers. A replay starts a new watch, so a refusal of an earlier finish + // that lands after it must not raise `finishRefused` for the finish that watch makes later — the + // leave then skipped its watched patch, and the cache said unwatched, mid-item, for an item the + // server had watched. + let finishGen = 0; // `final` = a settled checkpoint (pause / page-hide / leaving the player) vs. the periodic tick. // Only final checkpoints get mirrored to a linked Plex account (backend-side debounce), so Plex // sees the resume position when the user actually stops — not a timeline call every 10 seconds. @@ -261,6 +269,7 @@ export function usePlexProgress(opts: { const onPlayAgain = () => { finished = false; finishRefused = false; + finishGen = finishGen + 1; }; video?.addEventListener("play", onPlayAgain); // Back (unmount) persists the position server-side via flush(), but reopening the same item in @@ -273,12 +282,14 @@ export function usePlexProgress(opts: { const settle = () => { // Match the language-keyed detail query so the optimistic resume patch lands. const key = qk.plexItem(id, plexLang(i18n.language)); + left = true; if (finished) { // A refused finish claims nothing: the server has the item unwatched at the position the - // rollback restored, and the catch below has already dropped this entry, so the next open - // fetches the truth. Re-patching "watched / 0" here would put back the very lie the - // refusal handler removed. - if (!finishRefused) + // rollback restored. Re-patching "watched / 0" here would put back the very lie the refusal + // handler took back, and the entry the player kept observing (see the catch below) may + // still be the one from before the rollback — dropped, the next open fetches the truth. + if (finishRefused) qc.removeQueries({ queryKey: key }); + else qc.setQueryData(key, (old) => old ? { ...old, status: "watched", position_seconds: 0 } : old, ); @@ -293,6 +304,9 @@ export function usePlexProgress(opts: { settleRef.current = settle; finishRef.current = (confirmed?: Promise) => { finished = true; + finishRefused = false; + finishGen = finishGen + 1; + const gen = finishGen; // The latch has to go up BEFORE the media pauses, so it cannot wait for the server. If the // watched write is then refused, what is taken back is the CLAIM, not the latch (see // `finishRefused`): the cached detail said watched / position 0 while the server still had @@ -306,6 +320,7 @@ export function usePlexProgress(opts: { // position left an unwatched item with a ~99% bar that reopened in its last seconds. const ended = video?.ended === true; void confirmed?.catch(() => { + if (gen !== finishGen) return; finishRefused = true; // Restore the resume point the finish replaced — but ONLY where there is one to restore. // On the play-to-the-end path `pos` IS the end of the media, and the backend reads a @@ -316,10 +331,15 @@ export function usePlexProgress(opts: { // credits-skip caller captures a mid-episode position and is unaffected. if (!ended && pos > 0 && dur > 0 && pos <= dur - FINISH_MARGIN_S) api.plexProgress(id, pos, dur, true).catch(() => {}); - // `settle` may already have patched the detail to watched / 0 on the way out. Dropping that - // entry rather than invalidating it: an invalidated query with no observer keeps serving its - // stale data, and this one is a lie — no data is better, the next open fetches the truth. - qc.removeQueries({ queryKey: qk.plexItem(id, plexLang(i18n.language)) }); + // Still on the item (the up-next card is the default end state), the player observes this + // entry: emptying it dropped the card — gated on the detail — and its countdown restarted + // when the refetch landed, or the card never came back if that failed too. So it is + // refetched under the player, and dropped when the player leaves (`settle`). Already left, + // `settle` may have patched it to watched / 0 on the way out, and an invalidated query with + // no observer keeps serving that lie: dropped now, the next open fetches the truth. + const key = qk.plexItem(id, plexLang(i18n.language)); + if (left) qc.removeQueries({ queryKey: key }); + else void qc.invalidateQueries({ queryKey: key }); }); }; return () => { From f49dc6532fc5e4f0778dca189afcb203f562ccd7 Mon Sep 17 00:00:00 2001 From: npeter83 Date: Wed, 28 Oct 2026 20:01:21 +0100 Subject: [PATCH 05/22] feat(notifications): what asks for you comes first, then one stream; old read rows fold The center showed two fixed sections, the server's above this browser's, so an admin's "access requests waiting" sat under every server row, week-old read "Download ready" notices among them. One order now, for both sources (lib/inboxOrder): "Needs you" (unread, an access-requests nudge, a notice waiting on an interaction), then everything else newest first, and read rows older than a week fold under "Earlier (n)", closed by default. A row that asked in this visit keeps its place once read: marking it read no longer moves it to another list under the reader. Mark all read empties the band, the client notices of the visit included (the server rows read at once, not when the write lands). Marking a row read from its button hands the focus to the row instead of dropping it on as the button goes. --- frontend/src/i18n/locales/en/inbox.json | 7 +- frontend/src/i18n/locales/hu/inbox.json | 7 +- frontend/src/lib/inboxOrder.test.ts | 113 ++++++++++ frontend/src/lib/inboxOrder.ts | 72 ++++++ .../NotificationsPanel.order.dom.test.tsx | 212 ++++++++++++++++++ .../notifications/NotificationsPanel.tsx | 148 ++++++++---- 6 files changed, 507 insertions(+), 52 deletions(-) create mode 100644 frontend/src/lib/inboxOrder.test.ts create mode 100644 frontend/src/lib/inboxOrder.ts create mode 100644 frontend/src/modules/notifications/NotificationsPanel.order.dom.test.tsx diff --git a/frontend/src/i18n/locales/en/inbox.json b/frontend/src/i18n/locales/en/inbox.json index a34194aa..ca0d855b 100644 --- a/frontend/src/i18n/locales/en/inbox.json +++ b/frontend/src/i18n/locales/en/inbox.json @@ -9,8 +9,11 @@ "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..3b7b3f29 100644 --- a/frontend/src/i18n/locales/hu/inbox.json +++ b/frontend/src/i18n/locales/hu/inbox.json @@ -9,8 +9,11 @@ "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..262947eb --- /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. */ +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/modules/notifications/NotificationsPanel.order.dom.test.tsx b/frontend/src/modules/notifications/NotificationsPanel.order.dom.test.tsx new file mode 100644 index 00000000..d95166bf --- /dev/null +++ b/frontend/src/modules/notifications/NotificationsPanel.order.dom.test.tsx @@ -0,0 +1,212 @@ +// @vitest-environment jsdom +// +// The notification center's order: what asks for the reader comes first, whichever source it is +// from, then one time-ordered stream; read rows older than a week fold under "Earlier (n)". It was +// two fixed sections, the server's above this browser's, so an admin's "access requests waiting" +// sat under every server row — week-old read "Download ready" notices among them. +// i18n is key-passthrough, so labels read as keys. +import { afterEach, beforeEach, describe, expect, it, vi } from "vitest"; +import { act, cleanup, fireEvent, render, screen, waitFor, within } from "@testing-library/react"; +import { QueryClient, QueryClientProvider } from "@tanstack/react-query"; +import { api, type AppNotification } from "../../lib/api"; +import type { Notification as ClientNotif } from "../../lib/notifications"; + +const h = vi.hoisted(() => ({ + server: [] as unknown[], + client: [] as unknown[], +})); + +vi.mock("react-i18next", async (importOriginal) => ({ + ...(await importOriginal()), + useTranslation: () => ({ + i18n: { language: "en" }, + t: (key: string, o?: { count?: number }) => + o?.count === undefined ? key : `${key}:${o.count}`, + }), +})); +vi.mock("../../components/ui/confirmContext", () => ({ useConfirm: () => vi.fn() })); +vi.mock("../../lib/notifications", async (importOriginal) => ({ + ...(await importOriginal()), + subscribe: () => () => {}, + getNotifications: () => h.client, + getUnreadCount: () => 0, + markAllRead: () => {}, +})); +vi.mock("../../features/navigation/navigationContext", () => ({ + useNavigationActions: () => ({ setPage: vi.fn() }), +})); +vi.mock("../files/filesContext", () => ({ useFilesActions: () => ({ openFile: vi.fn() }) })); +vi.mock("../channels/channelsContext", () => ({ + useChannelsActions: () => ({ focusChannel: vi.fn() }), +})); +vi.mock("../feed/feedFiltersContext", () => ({ + useFeedFilters: () => ({}), + useFeedFiltersActions: () => ({ setFilters: vi.fn() }), +})); +vi.mock("../../components/ui/PageShell", async (importOriginal) => ({ + ...(await importOriginal()), + PageToolbar: ({ children }: { children: React.ReactNode }) => <>{children}, +})); + +import NotificationsPanel from "./NotificationsPanel"; + +// The server list goes through the real live query, so an optimistic patch of its cache is seen. +beforeEach(() => { + vi.spyOn(api, "notifications").mockImplementation(async () => ({ + items: h.server as AppNotification[], + total: h.server.length, + })); +}); +afterEach(() => { + cleanup(); + vi.restoreAllMocks(); +}); + +const H = 3600 * 1000; +const D = 24 * H; +const server = (id: number, ageMs: number, read: boolean) => + ({ + id, + type: "download_done", + title: `Server ${id}`, + body: null, + data: { title: `Ready ${id}` }, + read, + dismissed: false, + created_at: new Date(Date.now() - ageMs).toISOString(), + }) as unknown as AppNotification; +const client = (id: number, ageMs: number, extra: Partial = {}) => + ({ + id, + level: "success", + message: `Activity ${id}.`, + requiresInteraction: false, + ts: Date.now() - ageMs, + read: true, + dismissed: true, + transient: false, + repeat: 1, + ...extra, + }) as unknown as ClientNotif; + +function mount() { + const qc = new QueryClient(); + const tree = () => ( + + + + ); + const { rerender } = render(tree()); + return { rerender: () => rerender(tree()) }; +} +// Every test has a "Ready 1" on screen once the server list has landed. +const loaded = () => screen.findByText("Ready 1"); +// The rows of the list under a section's heading. +const listUnder = (heading: string) => + [...screen.getByText(heading).closest("section")!.querySelectorAll("[data-nav-index]")].map( + (r) => (r.textContent ?? "").match(/Ready \d+|Activity \d+/)?.[0], + ); +// Every row on screen, in document order, by its text. +const rows = () => + [...document.querySelectorAll("[data-nav-index]")].map( + (r) => (r.textContent ?? "").match(/Ready \d+|Activity \d+|common\.accessRequestsTitle/)?.[0], + ); + +describe("NotificationsPanel · order", () => { + it("puts a waiting access-requests nudge first, above every server row", async () => { + h.server = [server(1, 1 * H, false), server(2, 1 * D, true)]; + h.client = [ + client(9, 5 * 60 * 1000, { + level: "warning", + title: "common.accessRequestsTitle", + message: "1 pending", + meta: { kind: "access-requests" }, + }), + client(8, 2 * D), + ]; + mount(); + await loaded(); + expect(rows()).toEqual(["common.accessRequestsTitle", "Ready 1", "Ready 2", "Activity 8"]); + expect(screen.getByText("inbox.sectionNeedsYou:2")).toBeTruthy(); + expect(screen.getByText("inbox.sectionRecent")).toBeTruthy(); + }); + + it("folds read rows older than a week under Earlier (n), closed until opened", async () => { + h.server = [server(1, 1 * H, true), server(2, 10 * D, true)]; + h.client = [client(3, 9 * D)]; + mount(); + await loaded(); + expect(rows()).toEqual(["Ready 1"]); + const fold = screen.getByRole("button", { name: "inbox.earlier:2" }); + expect(fold.getAttribute("aria-expanded")).toBe("false"); + fireEvent.click(fold); + expect(fold.getAttribute("aria-expanded")).toBe("true"); + expect(rows()).toEqual(["Ready 1", "Activity 3", "Ready 2"]); + }); + + it("numbers each list's rows from 0 for the keyboard walk", async () => { + h.server = [server(1, 1 * H, false), server(2, 2 * H, true), server(3, 10 * D, true)]; + h.client = [client(4, 3 * H)]; + mount(); + await loaded(); + fireEvent.click(screen.getByRole("button", { name: "inbox.earlier:1" })); + const lists = [...document.querySelectorAll("[data-nav-list]")].map((l) => + [...l.querySelectorAll("[data-nav-index]")].map((r) => r.getAttribute("data-nav-index")), + ); + expect(lists).toEqual([["0"], ["0", "1"], ["0"]]); + }); + + it("a row marked read keeps its place in Needs you for the visit, in the same list", async () => { + vi.spyOn(api, "markNotificationRead").mockResolvedValue({ ok: true } as never); + h.server = [server(1, 1 * H, false), server(2, 2 * H, true)]; + h.client = []; + const { rerender } = mount(); + await loaded(); + const before = screen.getByText("Ready 1").closest("[data-nav-index]"); + fireEvent.click(within(before as HTMLElement).getByRole("button", { name: "inbox.markRead" })); + h.server = [server(1, 1 * H, true), server(2, 2 * H, true)]; // what the refetch after the write reads + await waitFor(() => + expect(screen.queryByRole("button", { name: "inbox.markRead" })).toBeNull(), + ); + await act(async () => rerender()); + expect(listUnder("inbox.sectionNeedsYou:1")).toEqual(["Ready 1"]); + // The same element, not a remount in another list: the keyboard focus stays on it. + expect(screen.getByText("Ready 1").closest("[data-nav-index]")).toBe(before); + }); + + it("Mark all read empties Needs you, the client notices of the visit included", async () => { + vi.spyOn(api, "markAllNotificationsRead").mockImplementation(async () => { + h.server = [server(1, 1 * H, true)]; + return { ok: true } as never; + }); + h.server = [server(1, 1 * H, false)]; + h.client = [client(2, 2 * H, { read: false })]; + const { rerender } = mount(); + await loaded(); + expect(listUnder("inbox.sectionNeedsYou:2")).toEqual(["Ready 1", "Activity 2"]); + h.client = [client(2, 2 * H, { read: true })]; // what the store's markAllRead leaves + fireEvent.click(screen.getByRole("button", { name: "inbox.markAllRead" })); + await act(async () => rerender()); + expect(screen.queryByText(/inbox\.sectionNeedsYou/)).toBeNull(); + await waitFor(() => expect(api.markAllNotificationsRead).toHaveBeenCalled()); + expect(listUnder("inbox.sectionRecent")).toEqual(["Ready 1", "Activity 2"]); + }); + + it("marking a row read from its button hands the focus to the row, not ", async () => { + vi.spyOn(api, "markNotificationRead").mockResolvedValue({ ok: true } as never); + h.server = [server(1, 1 * H, false)]; + h.client = []; + mount(); + await loaded(); + const row = screen.getByText("Ready 1").closest("[data-nav-index]")!; + const button = within(row).getByRole("button", { name: "inbox.markRead" }); + button.focus(); + h.server = [server(1, 1 * H, true)]; // what the refetch after the write reads + fireEvent.click(button); + await waitFor(() => + expect(screen.queryByRole("button", { name: "inbox.markRead" })).toBeNull(), + ); + expect(row.isConnected).toBe(true); + expect(document.activeElement).toBe(row); + }); +}); diff --git a/frontend/src/modules/notifications/NotificationsPanel.tsx b/frontend/src/modules/notifications/NotificationsPanel.tsx index 1151674a..25f3805a 100644 --- a/frontend/src/modules/notifications/NotificationsPanel.tsx +++ b/frontend/src/modules/notifications/NotificationsPanel.tsx @@ -1,4 +1,4 @@ -import { useEffect, useSyncExternalStore, type ReactNode } from "react"; +import { useEffect, useState, useSyncExternalStore, type ReactNode } from "react"; import { useTranslation } from "react-i18next"; import type { TFunction } from "i18next"; import { useMutation, useQueryClient } from "@tanstack/react-query"; @@ -8,6 +8,8 @@ import { Bell, Check, CheckCheck, + ChevronDown, + ChevronRight, ExternalLink, Eye, FolderOpen, @@ -41,6 +43,7 @@ import { type VideoWatchedMeta, } from "../../lib/notifications"; import { channelYouTubeUrl, formatDate } from "../../lib/format"; +import { entryKey, keyOf, orderInbox, type InboxEntry } from "../../lib/inboxOrder"; import { Button } from "../../components/ui/Button"; import { IconButton } from "../../components/ui/IconButton"; import { LEVEL_STYLE } from "../../components/ui/toastLevels"; @@ -55,7 +58,7 @@ import { useFeedFilters, useFeedFiltersActions } from "../feed"; import { SURFACE } from "../../components/ui/surfaces"; import { SectionLabel } from "../../components/ui/Headings"; import NavGrid from "../../components/ui/NavGrid"; -import { navItemAttrs } from "../../lib/listNav"; +import { focusNavItem, navItemAttrs } from "../../lib/listNav"; import { useContextMenu, type ContextMenuController } from "../../components/ui/useContextMenu"; import { ContextMenu } from "../../components/ui/ContextMenu"; import { MoreButton } from "../../components/ui/MoreButton"; @@ -65,10 +68,11 @@ import { HOVER_REVEAL } from "../../components/ui/reveal"; import { useConfirm } from "../../components/ui/confirmContext"; import { RelativeTime } from "../../components/ui/RelativeTime"; -// The single notification center. "System" = durable, server-backed notifications (inbox -// phase 1). "Activity" = the client-side transient events (action confirmations, errors, with -// their inline undo/find actions) that used to live behind a separate bell — folded in here so -// there's one indicator and one place to look. Transient toasts still pop via the Toaster. +// The single notification center, over two sources: the durable, server-backed notifications, and +// this browser's client-side events (action confirmations, errors, with their inline undo/find +// actions) that used to live behind a separate bell — folded in here so there's one indicator and +// one place to look. Transient toasts still pop via the Toaster. Both sources share one order +// (`lib/inboxOrder`): what asks for the reader first, then one stream, old read rows folded. const POLL_MS = 8000; const byId = (n: { id: number }) => n.id; @@ -93,6 +97,20 @@ export default function NotificationsPanel() { const clientItems = useSyncExternalStore(subscribe, getNotifications, getNotifications); const clientUnread = useSyncExternalStore(subscribe, getUnreadCount, getUnreadCount); + // The rows that asked for the reader in this visit keep their place in "Needs you" once read + // (`lib/inboxOrder`): seeded with the client notices unread at open, which the effect below marks + // read, and grown by every row that asks while the center is open. Mark all read empties it — the + // one gesture that means "I have seen the lot". The fold is measured from the time the center + // opened, fixed for the visit, so a row does not fold away under the reader. + const [held, setHeld] = useState>( + () => new Set(getNotifications().flatMap((n) => (n.read ? [] : [keyOf("client", n.id)]))), + ); + const [openedAt] = useState(() => Date.now()); + const [earlierOpen, setEarlierOpen] = useState(false); + const order = orderInbox(serverItems, clientItems, { now: openedAt, held }); + const asking = order.needsYou.map(entryKey).filter((k) => !held.has(k)); + if (asking.length > 0) setHeld(new Set([...held, ...asking])); + // Opening the center marks the (ephemeral) client items read, matching the old bell. Server // items stay unread until acted on individually. useEffect(() => { @@ -130,6 +148,13 @@ export default function NotificationsPanel() { function markAll() { markAllClientRead(); + // Read at once, not when the write lands: until then the rows still read unread, and the band + // would take them straight back. A failed write refetches the truth (refresh, then the poll). + void qc.cancelQueries({ queryKey: qk.notifications() }); + qc.setQueryData<{ items: AppNotification[]; total: number }>(qk.notifications(), (d) => + d ? { ...d, items: d.items.map((n) => (n.read ? n : { ...n, read: true })) } : d, + ); + setHeld(new Set()); if (serverUnread) readAllMut.mutate(); else refresh(); } @@ -340,6 +365,37 @@ export default function NotificationsPanel() { ); }; + // One row of either source; `i` is its place in the list it is rendered into. + const entry = (e: InboxEntry, i: number) => + e.src === "server" ? ( + readMut.mutate(e.n.id)} + onDismiss={() => dismissMut.mutate(e.n.id)} + onOpenScheduler={openScheduler} + onOpenTakeout={openTakeout} + onOpenFile={() => openDone(e.n)} + onOpenTokens={openTokens} + /> + ) : ( + removeClient(e.n.id)} + /> + ); + // --- The page's own menu: the header's "…" ------------------------------------------------------ const headerMenu = useContextMenu<"inbox">(); const headerItems: ActionMenuItem[] = [ @@ -409,51 +465,35 @@ export default function NotificationsPanel() { ) : (
- {serverItems.length > 0 && ( + {order.needsYou.length > 0 && (
- {t("inbox.sectionSystem")} - - {serverItems.map((n, i) => { - return ( - readMut.mutate(n.id)} - onDismiss={() => dismissMut.mutate(n.id)} - onOpenScheduler={openScheduler} - onOpenTakeout={openTakeout} - onOpenFile={() => openDone(n)} - onOpenTokens={openTokens} - /> - ); - })} - + + {t("inbox.sectionNeedsYou", { count: order.needsYou.length })} + + {order.needsYou.map(entry)}
)} - {clientItems.length > 0 && ( + {order.recent.length > 0 && (
- {t("inbox.sectionActivity")} - - {clientItems.map((n, i) => { - return ( - removeClient(n.id)} - /> - ); - })} - + {t("inbox.sectionRecent")} + {order.recent.map(entry)} +
+ )} + {order.earlier.length > 0 && ( +
+ + {earlierOpen && ( + {order.earlier.map(entry)} + )}
)}
@@ -517,7 +557,19 @@ function RowEnd({ return (
{onRead && ( - + { + // The button goes once the row reads read, and the focus on it would fall to : + // hand it to the row first, where j / k carry on. + const row = e.currentTarget.closest("[data-nav-index]"); + if (row?.parentElement && document.activeElement === e.currentTarget) + focusNavItem(row.parentElement, Number(row.getAttribute("data-nav-index"))); + onRead(); + }} + > )} From a5666bfde9c3e9418acc15fcf9050a5e8a4f90da Mon Sep 17 00:00:00 2001 From: npeter83 Date: Wed, 28 Oct 2026 20:42:18 +0100 Subject: [PATCH 06/22] style(notifications): the Earlier fold reads as a section heading, like its siblings It was a ghost button in body type under two uppercase section labels. It is now the heading itself, its text the disclosure's button (24px tall, past the WCAG 2.5.8 floor). --- .../NotificationsPanel.order.dom.test.tsx | 2 ++ .../notifications/NotificationsPanel.tsx | 27 ++++++++++++------- 2 files changed, 19 insertions(+), 10 deletions(-) diff --git a/frontend/src/modules/notifications/NotificationsPanel.order.dom.test.tsx b/frontend/src/modules/notifications/NotificationsPanel.order.dom.test.tsx index d95166bf..1d821ae9 100644 --- a/frontend/src/modules/notifications/NotificationsPanel.order.dom.test.tsx +++ b/frontend/src/modules/notifications/NotificationsPanel.order.dom.test.tsx @@ -139,6 +139,8 @@ describe("NotificationsPanel · order", () => { expect(rows()).toEqual(["Ready 1"]); const fold = screen.getByRole("button", { name: "inbox.earlier:2" }); expect(fold.getAttribute("aria-expanded")).toBe("false"); + // A section heading like "Needs you" and "Recent", its text the disclosure's button. + expect(fold.closest("h2")).not.toBeNull(); fireEvent.click(fold); expect(fold.getAttribute("aria-expanded")).toBe("true"); expect(rows()).toEqual(["Ready 1", "Activity 3", "Ready 2"]); diff --git a/frontend/src/modules/notifications/NotificationsPanel.tsx b/frontend/src/modules/notifications/NotificationsPanel.tsx index 25f3805a..162a07ec 100644 --- a/frontend/src/modules/notifications/NotificationsPanel.tsx +++ b/frontend/src/modules/notifications/NotificationsPanel.tsx @@ -46,6 +46,7 @@ import { channelYouTubeUrl, formatDate } from "../../lib/format"; import { entryKey, keyOf, orderInbox, type InboxEntry } from "../../lib/inboxOrder"; import { Button } from "../../components/ui/Button"; import { IconButton } from "../../components/ui/IconButton"; +import { Pressable } from "../../components/ui/Pressable"; import { LEVEL_STYLE } from "../../components/ui/toastLevels"; import { PageToolbar } from "../../components/ui/PageShell"; import { PageMenu } from "../../components/ui/PageMenu"; @@ -481,16 +482,22 @@ export default function NotificationsPanel() { )} {order.earlier.length > 0 && (
- + {/* A heading like its siblings, its text the disclosure's button (the ARIA + disclosure pattern); 24px tall so the target clears the WCAG 2.5.8 floor. */} + + setEarlierOpen((o) => !o)} + className="min-h-6 inline-flex items-center gap-1 uppercase hover:text-fg transition" + > + {earlierOpen ? ( + + ) : ( + + )} + {t("inbox.earlier", { count: order.earlier.length })} + + {earlierOpen && ( {order.earlier.map(entry)} )} From c3ad7684ea39c64f8f9042f015cbef0c2a8a1d44 Mon Sep 17 00:00:00 2001 From: npeter83 Date: Wed, 28 Oct 2026 20:44:01 +0100 Subject: [PATCH 07/22] test(ui): pin the notifications Earlier fold as a plain Pressable Its text is the disclosure's own label, not a title to select. --- .../src/components/ui/pressableSelectable.contract.test.ts | 4 ++++ 1 file changed, 4 insertions(+) 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"], From 52b93d001404b6f8bf9a6efbd46eecb9bd5c48aa Mon Sep 17 00:00:00 2001 From: npeter83 Date: Wed, 28 Oct 2026 21:53:59 +0100 Subject: [PATCH 08/22] fix(plex): a restart that fails to start still deletes the session it moved aside --- backend/app/plex/stream.py | 132 +++++++++++++++--------------- backend/tests/test_plex_stream.py | 60 ++++++++++++++ 2 files changed, 128 insertions(+), 64 deletions(-) diff --git a/backend/app/plex/stream.py b/backend/app/plex/stream.py index 0f957a03..7c4ec00a 100644 --- a/backend/app/plex/stream.py +++ b/backend/app/plex/stream.py @@ -820,74 +820,78 @@ def start_session( # 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 doomed: list[tuple[HlsSession, Path | 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: - 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, + # 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. - doomed += _enforce_caps( - user_id, - max(1, sysconfig.get_int(db, "plex_max_transcodes")), - max(1, sysconfig.get_int(db, "plex_max_sessions")), - ) - _finish(doomed) + 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)) diff --git a/backend/tests/test_plex_stream.py b/backend/tests/test_plex_stream.py index 9d56e8ca..379d02e1 100644 --- a/backend/tests/test_plex_stream.py +++ b/backend/tests/test_plex_stream.py @@ -1058,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) == [] From ee658b8bfc529f56a25d90d832978a568ab968e8 Mon Sep 17 00:00:00 2001 From: npeter83 Date: Wed, 28 Oct 2026 21:53:59 +0100 Subject: [PATCH 09/22] test(plex): pin the late-checkpoint race, its window, and the bulk mark's push supersede --- backend/tests/test_plex_finish.py | 92 ++++++++++++++++++++++++++++++- 1 file changed, 91 insertions(+), 1 deletion(-) diff --git a/backend/tests/test_plex_finish.py b/backend/tests/test_plex_finish.py index f264ba57..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 @@ -138,3 +138,93 @@ def test_marking_watched_supersedes_a_resume_push_still_in_flight( 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 From 0469812499c213e63be6e480fbca2a9248af8a7a Mon Sep 17 00:00:00 2001 From: npeter83 Date: Wed, 28 Oct 2026 21:54:00 +0100 Subject: [PATCH 10/22] fix(notifications): Mark all read empties a band of read rows, and a read landing mid-write cannot refill it --- .../NotificationsPanel.order.dom.test.tsx | 57 ++++++++++++++++++- .../notifications/NotificationsPanel.tsx | 26 ++++++--- 2 files changed, 74 insertions(+), 9 deletions(-) diff --git a/frontend/src/modules/notifications/NotificationsPanel.order.dom.test.tsx b/frontend/src/modules/notifications/NotificationsPanel.order.dom.test.tsx index 1d821ae9..0f489b27 100644 --- a/frontend/src/modules/notifications/NotificationsPanel.order.dom.test.tsx +++ b/frontend/src/modules/notifications/NotificationsPanel.order.dom.test.tsx @@ -97,7 +97,7 @@ function mount() { ); const { rerender } = render(tree()); - return { rerender: () => rerender(tree()) }; + return { qc, rerender: () => rerender(tree()) }; } // Every test has a "Ready 1" on screen once the server list has landed. const loaded = () => screen.findByText("Ready 1"); @@ -211,4 +211,59 @@ describe("NotificationsPanel · order", () => { expect(row.isConnected).toBe(true); expect(document.activeElement).toBe(row); }); + + it("a band of rows already read still offers Mark all read, which empties it", async () => { + // Opening the center marked this browser's notices read: the band holds them, and nothing on + // the page is unread any more. + h.server = [server(1, 1 * H, true)]; + h.client = [client(2, 2 * H, { read: false })]; + const { rerender } = mount(); + await loaded(); + h.client = [client(2, 2 * H, { read: true })]; // what the mount's markAllRead leaves + await act(async () => rerender()); + expect(listUnder("inbox.sectionNeedsYou:1")).toEqual(["Activity 2"]); + const button = screen.getByRole("button", { name: "inbox.markAllRead" }); + expect(button.hasAttribute("disabled")).toBe(false); + fireEvent.click(button); + await act(async () => rerender()); + expect(screen.queryByText(/inbox\.sectionNeedsYou/)).toBeNull(); + }); + + it("a read that lands before Mark all read commits does not put the rows back", async () => { + let commit!: () => void; + vi.spyOn(api, "markAllNotificationsRead").mockImplementation( + () => new Promise((resolve) => (commit = () => resolve({ ok: true } as never))), + ); + h.server = [server(1, 1 * H, false)]; + h.client = []; + const { qc } = mount(); + await loaded(); + fireEvent.click(screen.getByRole("button", { name: "inbox.markAllRead" })); + await waitFor(() => expect(api.markAllNotificationsRead).toHaveBeenCalled()); + // The poll (or another row's write) reads the list before the server has committed the write. + void qc.invalidateQueries({ queryKey: ["notifications"] }); + await waitFor(() => + expect(qc.getQueryData<{ items: AppNotification[] }>(["notifications"])?.items[0]?.read).toBe( + false, + ), + ); + h.server = [server(1, 1 * H, true)]; + await act(async () => commit()); + await waitFor(() => expect(screen.queryByText(/inbox\.sectionNeedsYou/)).toBeNull()); + expect(listUnder("inbox.sectionRecent")).toEqual(["Ready 1"]); + }); + + it("a refused Mark all read brings the rows back at once, not at the next poll", async () => { + vi.spyOn(api, "markAllNotificationsRead").mockRejectedValue(new Error("offline")); + h.server = [server(1, 1 * H, false)]; + h.client = []; + mount(); + await loaded(); + const reads = vi.mocked(api.notifications).mock.calls.length; + fireEvent.click(screen.getByRole("button", { name: "inbox.markAllRead" })); + await waitFor(() => + expect(vi.mocked(api.notifications).mock.calls.length).toBeGreaterThan(reads), + ); + await waitFor(() => expect(listUnder("inbox.sectionNeedsYou:1")).toEqual(["Ready 1"])); + }); }); diff --git a/frontend/src/modules/notifications/NotificationsPanel.tsx b/frontend/src/modules/notifications/NotificationsPanel.tsx index 162a07ec..a02c8b44 100644 --- a/frontend/src/modules/notifications/NotificationsPanel.tsx +++ b/frontend/src/modules/notifications/NotificationsPanel.tsx @@ -108,9 +108,6 @@ export default function NotificationsPanel() { ); const [openedAt] = useState(() => Date.now()); const [earlierOpen, setEarlierOpen] = useState(false); - const order = orderInbox(serverItems, clientItems, { now: openedAt, held }); - const asking = order.needsYou.map(entryKey).filter((k) => !held.has(k)); - if (asking.length > 0) setHeld(new Set([...held, ...asking])); // Opening the center marks the (ephemeral) client items read, matching the old bell. Server // items stay unread until acted on individually. @@ -129,7 +126,9 @@ export default function NotificationsPanel() { }); const readAllMut = useMutation({ mutationFn: api.markAllNotificationsRead, - onSuccess: refresh, + // Settled, not only succeeded: a refused write brings the rows back unread at once, rather than + // leaving them read until the next poll moves them under the reader. + onSettled: refresh, meta: { failTitle: "errors.writeFailed.notifReadAll" }, }); const dismissMut = useMutation({ @@ -143,14 +142,25 @@ export default function NotificationsPanel() { meta: { failTitle: "errors.writeFailed.notifClear" }, }); + // While Mark all read is on its way, a read that lands before the server commits it (the poll, or + // another row's write refreshing the list) still says "unread": those rows are read already, or + // the band would take them back and hold them for the rest of the visit. + const shownServer = readAllMut.isPending + ? serverItems.map((n) => (n.read ? n : { ...n, read: true })) + : serverItems; + const order = orderInbox(shownServer, clientItems, { now: openedAt, held }); + const asking = order.needsYou.map(entryKey).filter((k) => !held.has(k)); + if (asking.length > 0) setHeld(new Set([...held, ...asking])); + const serverUnread = serverItems.some((n) => !n.read); const hasAny = serverItems.length > 0 || clientItems.length > 0; - const hasUnread = serverUnread || clientUnread > 0; + // A band of rows already read still offers the gesture that empties it. + const canMarkAll = serverUnread || clientUnread > 0 || order.needsYou.length > 0; function markAll() { markAllClientRead(); // Read at once, not when the write lands: until then the rows still read unread, and the band - // would take them straight back. A failed write refetches the truth (refresh, then the poll). + // would take them straight back. A failed write refetches the truth when it settles. void qc.cancelQueries({ queryKey: qk.notifications() }); qc.setQueryData<{ items: AppNotification[]; total: number }>(qk.notifications(), (d) => d ? { ...d, items: d.items.map((n) => (n.read ? n : { ...n, read: true })) } : d, @@ -404,7 +414,7 @@ export default function NotificationsPanel() { id: "read-all", label: t("inbox.markAllRead"), icon: icon(CheckCheck), - disabled: !hasUnread || readAllMut.isPending, + disabled: !canMarkAll || readAllMut.isPending, onSelect: markAll, }, { @@ -438,7 +448,7 @@ export default function NotificationsPanel() { variant="ghost" size="sm" onClick={markAll} - disabled={!hasUnread || readAllMut.isPending} + disabled={!canMarkAll || readAllMut.isPending} icon={} > {t("inbox.markAllRead")} From c86ec7f7a502e4b031f30054f1149c1dc92bd21d Mon Sep 17 00:00:00 2001 From: npeter83 Date: Wed, 28 Oct 2026 21:54:00 +0100 Subject: [PATCH 11/22] fix(nav): Back finds an item that moved to another list by its name --- .../src/lib/listNav.restoreMoved.dom.test.tsx | 75 +++++++++++++++++++ frontend/src/lib/listNav.ts | 36 ++++++--- 2 files changed, 99 insertions(+), 12 deletions(-) create mode 100644 frontend/src/lib/listNav.restoreMoved.dom.test.tsx 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..a759247a --- /dev/null +++ b/frontend/src/lib/listNav.restoreMoved.dom.test.tsx @@ -0,0 +1,75 @@ +// @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 } from "vitest"; +import { act, cleanup, render } from "@testing-library/react"; +import { useRef } from "react"; + +import { navItemAttrs, navPosition, restoreNavPosition, useListNav } from "./listNav"; + +afterEach(cleanup); + +function List({ items }: { items: string[] }) { + const ref = useRef(null); + useListNav(ref, { enabled: true }); + return ( +
+ {items.map((t, i) => ( +
+ +
+ ))} +
+ ); +} + +const Page = ({ lists }: { lists: string[][] }) => ( +
+ {lists.map((items, i) => ( + + ))} +
+); + +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[][]) { + 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"); + }); +}); diff --git a/frontend/src/lib/listNav.ts b/frontend/src/lib/listNav.ts index 6d688fea..1ceaf0cf 100644 --- a/frontend/src/lib/listNav.ts +++ b/frontend/src/lib/listNav.ts @@ -460,17 +460,28 @@ export function navPosition(): NavPosition | null { const RESTORE_MS = 2000; const RESTORE_POLL_MS = 50; -/** The item in `list` to hand the focus back to: the saved place while it still holds the saved +/** The item to hand the focus back to: in `list`, the saved place while it still holds the saved * name, else the nearest item of that name (names repeat — a log console's identical request - * lines), else the saved place. */ -function restoreTarget(list: NavList, pos: NavPosition): HTMLElement | null { - const at = itemAt(list, pos.index); - if (!pos.name || (at && itemName(at) === pos.name)) return at; - const distance = (el: HTMLElement) => Math.abs(indexOf(el) - pos.index); - const named = ownItems(list.el) - .filter((it) => itemName(it) === pos.name) - .sort((a, b) => distance(a) - distance(b)); - return named[0] ?? at; + * lines). Then the one item of that name in another of the page's lists: an item can change lists + * while the page was away (a notification asked for the reader on the visit it was opened from and + * sits in the stream on the next), and the place left behind is now its neighbour's. Only when the + * name is the page's alone — of two, either could be the one. Else the saved place. */ +function restoreTarget( + all: readonly NavList[], + list: NavList | undefined, + pos: NavPosition, +): HTMLElement | null { + const at = list ? itemAt(list, pos.index) : null; + if (list && (!pos.name || (at && itemName(at) === pos.name))) return at; + if (!pos.name) return null; + const named = (l: NavList) => ownItems(l.el).filter((it) => itemName(it) === pos.name); + if (list) { + const distance = (el: HTMLElement) => Math.abs(indexOf(el) - pos.index); + const here = named(list).sort((a, b) => distance(a) - distance(b)); + if (here[0]) return here[0]; + } + const [only, ...more] = all.flatMap(named); + return only && more.length === 0 ? only : at; } /** Focus the item at `pos` once its list is on screen again. A NAMED list (a rail) is found by its @@ -478,7 +489,8 @@ function restoreTarget(list: NavList, pos: NavPosition): HTMLElement | null { * watched in between, after the page's cache expired — shifts every place after it, and matching by * place landed the focus in the wrong rail. An unnamed list only by its * place, and only once the page has exactly as many lists as when it was left: with more or fewer, - * list N may be another list, and is the honest outcome. Gives up when the time is up, and at + * list N may be another list, and is the honest outcome — unless the item's name is the + * page's alone (`restoreTarget`). Gives up when the time is up, and at * once when focus is already somewhere (the reader moved on, a dialog took it). The item is focused * where the scroll restore put it, never scrolled to. (A stand-in the focus keeper placed when the view * it held was swapped away counts as untouched, like .) Returns a cancel. */ @@ -499,7 +511,7 @@ export function restoreNavPosition(pos: NavPosition): () => void { // The saved place still holding the saved name wins, and among several items of that name the // nearest one: names repeat (a log console's `http.request GET /api/feed 200` lines, two // downloads of one video), and the first of them is rarely the one left behind. - const el = list ? restoreTarget(list, pos) : null; + const el = restoreTarget(all, list, pos); if (el) return focusItem(el, false); if (Date.now() < deadline) timer = setTimeout(attempt, RESTORE_POLL_MS); }; From 25ad2ee1ba9390133050ee74cfae2238c144c268 Mon Sep 17 00:00:00 2001 From: npeter83 Date: Wed, 28 Oct 2026 21:54:00 +0100 Subject: [PATCH 12/22] test(ui): a self-closing dialog leaves the focus in a menu that stays open --- frontend/src/components/ui/Modal.dom.test.tsx | 30 +++++++++++++++++++ 1 file changed, 30 insertions(+) diff --git a/frontend/src/components/ui/Modal.dom.test.tsx b/frontend/src/components/ui/Modal.dom.test.tsx index 85facf5c..4169ed69 100644 --- a/frontend/src/components/ui/Modal.dom.test.tsx +++ b/frontend/src/components/ui/Modal.dom.test.tsx @@ -375,6 +375,36 @@ describe("Modal focus trap (offsetParent stubbed 'visible')", () => { 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. From 9dd56e94d4e0f2e6043e868677fcec4ee83390ba Mon Sep 17 00:00:00 2001 From: npeter83 Date: Wed, 28 Oct 2026 21:54:00 +0100 Subject: [PATCH 13/22] docs(design): the notification center's order and its Earlier fold --- docs/DESIGN.md | 6 +++++- 1 file changed, 5 insertions(+), 1 deletion(-) diff --git a/docs/DESIGN.md b/docs/DESIGN.md index e81a0579..f740a03e 100644 --- a/docs/DESIGN.md +++ b/docs/DESIGN.md @@ -378,7 +378,10 @@ 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. 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 +827,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. Not chosen: one stream with no band, this browser's activity above the server's, a server-side retention | From d825aff30da7d63fa0e5588af5d0ceebbe5ef9dc Mon Sep 17 00:00:00 2001 From: npeter83 Date: Wed, 28 Oct 2026 22:04:06 +0100 Subject: [PATCH 14/22] fix(notifications): a row is named by what it is about, not its kind alone --- .../NotificationsPanel.order.dom.test.tsx | 23 ++++++++++++++++++- .../notifications/NotificationsPanel.tsx | 6 +++-- 2 files changed, 26 insertions(+), 3 deletions(-) diff --git a/frontend/src/modules/notifications/NotificationsPanel.order.dom.test.tsx b/frontend/src/modules/notifications/NotificationsPanel.order.dom.test.tsx index 0f489b27..1e3ebc2c 100644 --- a/frontend/src/modules/notifications/NotificationsPanel.order.dom.test.tsx +++ b/frontend/src/modules/notifications/NotificationsPanel.order.dom.test.tsx @@ -223,7 +223,10 @@ describe("NotificationsPanel · order", () => { await act(async () => rerender()); expect(listUnder("inbox.sectionNeedsYou:1")).toEqual(["Activity 2"]); const button = screen.getByRole("button", { name: "inbox.markAllRead" }); - expect(button.hasAttribute("disabled")).toBe(false); + // Unavailable reads either way: natively while unfocused, aria-disabled while it holds the focus. + expect(button.hasAttribute("disabled") || button.getAttribute("aria-disabled") === "true").toBe( + false, + ); fireEvent.click(button); await act(async () => rerender()); expect(screen.queryByText(/inbox\.sectionNeedsYou/)).toBeNull(); @@ -266,4 +269,22 @@ describe("NotificationsPanel · order", () => { ); await waitFor(() => expect(listUnder("inbox.sectionNeedsYou:1")).toEqual(["Ready 1"])); }); + + it("names each row by what it is about, so two of one kind tell apart", async () => { + // A screen reader hears the name while the keyboard is on the row, and Back finds the row it + // left by it: "Download ready" alone was every download's name. + h.server = [server(1, 1 * H, true), server(2, 2 * H, true)]; + h.client = [client(3, 3 * H, { title: "Something broke", message: "Unexpected error." })]; + mount(); + await loaded(); + expect( + [...document.querySelectorAll("[data-nav-index]")].map((r) => + r.getAttribute("data-nav-name"), + ), + ).toEqual([ + "inbox.downloadDone.title: Ready 1", + "inbox.downloadDone.title: Ready 2", + "Something broke: Unexpected error.", + ]); + }); }); diff --git a/frontend/src/modules/notifications/NotificationsPanel.tsx b/frontend/src/modules/notifications/NotificationsPanel.tsx index a02c8b44..3ac8fae9 100644 --- a/frontend/src/modules/notifications/NotificationsPanel.tsx +++ b/frontend/src/modules/notifications/NotificationsPanel.tsx @@ -690,7 +690,9 @@ function NotificationRow({ } return (
From 22d873ed490d864bcbd1dfd77c37cfea838c9279 Mon Sep 17 00:00:00 2001 From: npeter83 Date: Wed, 28 Oct 2026 22:04:07 +0100 Subject: [PATCH 15/22] fix(nav): an item Back finds in another list is brought into view --- .../src/lib/listNav.restoreMoved.dom.test.tsx | 19 ++++++++++++++++++- frontend/src/lib/listNav.ts | 7 +++++-- 2 files changed, 23 insertions(+), 3 deletions(-) diff --git a/frontend/src/lib/listNav.restoreMoved.dom.test.tsx b/frontend/src/lib/listNav.restoreMoved.dom.test.tsx index a759247a..389f2124 100644 --- a/frontend/src/lib/listNav.restoreMoved.dom.test.tsx +++ b/frontend/src/lib/listNav.restoreMoved.dom.test.tsx @@ -5,7 +5,7 @@ // 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 } from "vitest"; +import { afterEach, describe, expect, it, vi } from "vitest"; import { act, cleanup, render } from "@testing-library/react"; import { useRef } from "react"; @@ -72,4 +72,21 @@ describe("Back to an item that moved to another list", () => { ]); 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; + await leaveAndReturn([["subscribed", "download"], ["older"]], "subscribed", [ + ["download"], + ["subscribed", "older"], + ]); + expect(reveal).toHaveBeenCalledTimes(1); + cleanup(); + reveal.mockClear(); + await leaveAndReturn([["subscribed", "download"]], "download", [["download", "subscribed"]]); + expect(focused()).toBe("download"); + expect(reveal).not.toHaveBeenCalled(); + }); }); diff --git a/frontend/src/lib/listNav.ts b/frontend/src/lib/listNav.ts index 1ceaf0cf..b2db0268 100644 --- a/frontend/src/lib/listNav.ts +++ b/frontend/src/lib/listNav.ts @@ -492,7 +492,8 @@ function restoreTarget( * list N may be another list, and is the honest outcome — unless the item's name is the * page's alone (`restoreTarget`). Gives up when the time is up, and at * once when focus is already somewhere (the reader moved on, a dialog took it). The item is focused - * where the scroll restore put it, never scrolled to. (A stand-in the focus keeper placed when the view + * where the scroll restore put it, never scrolled to — but one found in another list is brought into + * view. (A stand-in the focus keeper placed when the view * it held was swapped away counts as untouched, like .) Returns a cancel. */ export function restoreNavPosition(pos: NavPosition): () => void { const deadline = Date.now() + RESTORE_MS; @@ -512,7 +513,9 @@ export function restoreNavPosition(pos: NavPosition): () => void { // nearest one: names repeat (a log console's `http.request GET /api/feed 200` lines, two // downloads of one video), and the first of them is rarely the one left behind. const el = restoreTarget(all, list, pos); - if (el) return focusItem(el, false); + // Where the scroll restore put it — unless it is found in another list than the one it was left + // in: the restored scroll is the old place's, and the item would hold the focus off screen. + if (el) return focusItem(el, !list?.el.contains(el)); if (Date.now() < deadline) timer = setTimeout(attempt, RESTORE_POLL_MS); }; // Not inside the popstate dispatch that landed: a sub-view's own listener (`useHistorySubview`) From 947efaba6e8358a509c50762680b248298e34989 Mon Sep 17 00:00:00 2001 From: npeter83 Date: Thu, 29 Oct 2026 00:38:34 +0100 Subject: [PATCH 16/22] docs(design): the notification order's decision row carries what it was chosen over and why --- docs/DESIGN.md | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/docs/DESIGN.md b/docs/DESIGN.md index f740a03e..7273b150 100644 --- a/docs/DESIGN.md +++ b/docs/DESIGN.md @@ -827,4 +827,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. Not chosen: one stream with no band, this browser's activity above the server's, a server-side retention | +| 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 | From fa271a31b9ae822f94813781da3da6b0373c53da Mon Sep 17 00:00:00 2001 From: npeter83 Date: Thu, 29 Oct 2026 00:38:34 +0100 Subject: [PATCH 17/22] fix(notifications): Mark all read waits for the fresh list, is not offered where it cannot act, and a row says it is unread The mask over a stale read lifted when the write settled, before the refetch landed, and the band took the rows back. A band holding only what asks by its kind (an access-requests nudge) left the button enabled with nothing to do. The unread dot was only drawn: the spoken label now says it, apart from the name Back finds the row by, and follows the row while it holds the focus. --- frontend/src/i18n/locales/en/inbox.json | 1 + frontend/src/i18n/locales/hu/inbox.json | 1 + frontend/src/lib/inboxOrder.ts | 2 +- frontend/src/lib/listNav.ts | 32 ++++++- .../NotificationsPanel.order.dom.test.tsx | 92 +++++++++++++++++++ .../notifications/NotificationsPanel.tsx | 28 ++++-- 6 files changed, 144 insertions(+), 12 deletions(-) diff --git a/frontend/src/i18n/locales/en/inbox.json b/frontend/src/i18n/locales/en/inbox.json index ca0d855b..76680a88 100644 --- a/frontend/src/i18n/locales/en/inbox.json +++ b/frontend/src/i18n/locales/en/inbox.json @@ -6,6 +6,7 @@ "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", diff --git a/frontend/src/i18n/locales/hu/inbox.json b/frontend/src/i18n/locales/hu/inbox.json index 3b7b3f29..a50c51fc 100644 --- a/frontend/src/i18n/locales/hu/inbox.json +++ b/frontend/src/i18n/locales/hu/inbox.json @@ -6,6 +6,7 @@ "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", diff --git a/frontend/src/lib/inboxOrder.ts b/frontend/src/lib/inboxOrder.ts index 262947eb..4fdfb65d 100644 --- a/frontend/src/lib/inboxOrder.ts +++ b/frontend/src/lib/inboxOrder.ts @@ -38,7 +38,7 @@ 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. */ -const clientAsks = (n: ClientNotif): boolean => +export const clientAsks = (n: ClientNotif): boolean => n.meta?.kind === "access-requests" || (n.requiresInteraction && !n.dismissed); export function orderInbox( diff --git a/frontend/src/lib/listNav.ts b/frontend/src/lib/listNav.ts index b2db0268..ed1b241e 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,15 @@ 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); + 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 ); + 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(); + } + }); }); From 7489cab3bc328e5ac3c658920cd44852a98ac17b Mon Sep 17 00:00:00 2001 From: npeter83 Date: Thu, 29 Oct 2026 02:06:52 +0100 Subject: [PATCH 22/22] chore(release): 0.114.0 --- CHANGELOG.md | 11 +++++++++++ VERSION | 2 +- frontend/src/content/release-notes/en.json | 11 +++++++++++ frontend/src/content/release-notes/hu.json | 11 +++++++++++ 4 files changed, 34 insertions(+), 1 deletion(-) 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/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",