diff --git a/backend/app/debugsession.py b/backend/app/debugsession.py index 829fcb96..bf459014 100644 --- a/backend/app/debugsession.py +++ b/backend/app/debugsession.py @@ -21,6 +21,7 @@ import secrets from datetime import datetime, timedelta, timezone from enum import StrEnum from typing import Literal, get_args +from uuid import uuid4 from sqlalchemy import and_, delete, func, not_, select, update from sqlalchemy.orm import Session, defer @@ -38,6 +39,11 @@ SCREENSHOT_USER_MAX_BYTES = 100 * 1024 * 1024 # The screenshot types kept, and the extension each is exported and sent under — one table, so an # accepted type always has a name. IMAGE_TYPES = {"image/png": "png", "image/jpeg": "jpg", "image/webp": "webp"} +# What a screenshot is called in the zip and on the issue. `issues/bundle.py` builds the names from +# this and `IMAGE_TYPES`; this module and `issues/gitea.py` read them back to tell a screenshot's +# link from a log's. One definition, because three copies of a name is how the next rename breaks +# `images_pending` into silence. +SCREENSHOT_PREFIX = "screenshot-" # Rows an issue can carry. A 60-minute session at Teljes is a few thousand rows; past this the # bundle says how many it left out rather than growing without bound. BUNDLE_MAX = 5_000 @@ -180,7 +186,7 @@ def images_pending(session: DebugSession) -> bool: (every upload went, the edit that shows them did not).""" state = delivery(session) shown = state.get("images_shown") - return not shown and any(n.startswith("screenshot-") for n in state.get("links") or {}) + return not shown and any(n.startswith(SCREENSHOT_PREFIX) for n in state.get("links") or {}) def unfinished(session: DebugSession) -> bool: @@ -190,33 +196,63 @@ def unfinished(session: DebugSession) -> bool: # A send claims the session while it talks to the tracker, instead of holding the row's lock across # those calls: a Stop, an autosave or a screenshot would queue behind a lock for the whole send. -# They see the claim and are refused at once instead. A claim older than this was left by a request -# that died mid-send (the process restarted), and the next send takes it over. +# They see the claim and are refused at once instead. A claim not renewed for this long was left by +# a request that died mid-send (the process restarted), and the next send takes it over — which is +# why a live send RENEWS it every time it reports progress (`set_delivery`). A claim is a lease, and +# a send slower than the lease must not lose it while it is still running. SEND_CLAIM_SECONDS = 600 +def _claim(session: DebugSession) -> dict: + claim = delivery(session).get("sending") + return claim if isinstance(claim, dict) else {} + + +def _stamp(token: str) -> dict: + return {"at": datetime.now(timezone.utc).isoformat(), "by": token} + + def sending(session: DebugSession) -> bool: """Whether a send is under way right now (a live claim).""" - since = delivery(session).get("sending") - if not since: + at = _claim(session).get("at") + if not at: return False - started = datetime.fromisoformat(since) - return (datetime.now(timezone.utc) - started).total_seconds() < SEND_CLAIM_SECONDS + return (datetime.now(timezone.utc) - datetime.fromisoformat(at)).total_seconds() < ( + SEND_CLAIM_SECONDS + ) -def claim_send(session: DebugSession) -> None: - """Mark the session as being sent. The caller holds the lock and commits, which is what makes - the claim visible to everyone else.""" - set_delivery(session, {**delivery(session), "sending": datetime.now(timezone.utc).isoformat()}) +def claim_send(session: DebugSession) -> str: + """Mark the session as being sent, and return the token that OWNS this claim. The caller holds + the lock and commits, which is what makes the claim visible to everyone else.""" + token = uuid4().hex + _write_delivery(session, {**delivery(session), "sending": _stamp(token)}) + return token -def release_send(session: DebugSession) -> None: - state = {k: v for k, v in delivery(session).items() if k != "sending"} - set_delivery(session, state) +def release_send(session: DebugSession, token: str) -> None: + """Give back our OWN claim, and only ours: one that aged out and was taken over belongs to the + send holding it now, and handing that one back would let a third send in beside it.""" + if _claim(session).get("by") != token: + return + _write_delivery(session, {k: v for k, v in delivery(session).items() if k != "sending"}) def set_delivery(session: DebugSession, state: dict) -> None: - """Write a delivery's progress (app/issues/gitea.py keeps it; this module reads it).""" + """Write a delivery's progress (app/issues/gitea.py keeps it; this module reads it). + + The claim is not the caller's to carry. `gitea.deliver` threads ONE snapshot of this dict + through a whole send, so a progress report that carried the claim wrote a stale one back at + every step, and a send longer than the lease then expired while still running. It is taken from + the row and re-stamped here instead: a send still reporting progress is still alive.""" + claim = _claim(session) + state = {k: v for k, v in state.items() if k != "sending"} + if claim: + state["sending"] = _stamp(claim["by"]) + _write_delivery(session, state) + + +def _write_delivery(session: DebugSession, state: dict) -> None: # A new dict, so the JSONB column sees the change. session.draft = {**(session.draft or {}), "delivery": state} @@ -233,9 +269,13 @@ def serialize(session: DebugSession | None, screenshots: int = 0) -> dict | None "ended_reason": session.ended_reason, "issue_url": session.issue_url, # A send that stopped half-way: the files it has still to upload, and whether the issue's - # body has still to be edited to show the screenshots it already carries. + # body has still to be edited to show the screenshots it already carries. Both are true of a + # send that is RUNNING too — every step commits its progress — so `sending` goes with them: + # without it a reload or a second tab renders a live send as a failed one, and offers a + # retry that is certain to be refused. "pending_files": not_uploaded(session), "images_pending": images_pending(session), + "sending": sending(session), "screenshots": screenshots, "draft": session.draft, } @@ -275,6 +315,14 @@ def sniff(data: bytes) -> str | None: return None +def lock_user(db: Session, user_id: int) -> None: + """Hold this user's own lock for the rest of the transaction. A session's row lock bounds what + THAT session may keep; the byte cap below is the user's across all of them, and a lock on one + session cannot serialize a check that reads them all — two uploads to two sessions both read the + total before either commits, and both pass.""" + db.execute(select(func.pg_advisory_xact_lock(user_id))) + + def stored_screenshot_bytes(db: Session, user_id: int) -> int: return ( db.scalar( @@ -295,15 +343,17 @@ class ScreenshotRefused(Exception): def add_screenshot(db: Session, session: DebugSession, data: bytes) -> DebugScreenshot: - """Keep one screenshot. Locks the session and flushes; the caller commits. Refused once the - issue was sent (it can no longer carry it), past the per-session count or the user's storage, - or when it is not an image we can show.""" + """Keep one screenshot. Locks the user and the session, and flushes; the caller commits. + Refused while a send is under way or once the issue was sent (it can no longer carry it), past + the per-session count or the user's storage, or when it is not an image we can show.""" if not data: raise ScreenshotRefused("empty") mime = sniff(data) if mime is None: raise ScreenshotRefused("not_an_image") - # Under the lock: two uploads at once cannot both pass the count. + # Under both locks: two uploads at once cannot both pass the count (the session's), nor both + # pass the user's byte cap from two different sessions (the user's). Always in this order. + lock_user(db, session.user_id) lock(db, session) if sending(session): raise ScreenshotRefused("sending") diff --git a/backend/app/issues/bundle.py b/backend/app/issues/bundle.py index 3484f3ef..45c9def0 100644 --- a/backend/app/issues/bundle.py +++ b/backend/app/issues/bundle.py @@ -64,7 +64,8 @@ def files(db: Session, session: DebugSession) -> list[IssueFile]: ) for n, shot in enumerate(shots, start=1): ext = debugsession.IMAGE_TYPES[shot.mime] - out.append(IssueFile(f"screenshot-{n}.{ext}", shot.data, shot.mime)) + name = f"{debugsession.SCREENSHOT_PREFIX}{n}.{ext}" + out.append(IssueFile(name, shot.data, shot.mime)) return out diff --git a/backend/app/issues/gitea.py b/backend/app/issues/gitea.py index ecb7a53b..a103c422 100644 --- a/backend/app/issues/gitea.py +++ b/backend/app/issues/gitea.py @@ -20,9 +20,10 @@ Delivery keeps its progress in the session's draft (`delivery`: the issue number upload, the links of those uploaded, whether the body shows the screenshots yet), so a failure half-way keeps the issue that exists and a retry does only what is missing — never a second issue. Every step is checkpointed (the caller's `db.commit`), so what did happen is kept whatever fails -next — and no transaction is held open across these calls. The caller claims the session for the -send first (`debugsession.claim_send`), which is what keeps a second send, an autosave or a -screenshot out of the way. `debugsession.not_uploaded` / `images_pending` read that progress. +next — and the first checkpoint comes before the first call, so no row lock and no read snapshot is +held across them. The caller claims the session for the send first (`debugsession.claim_send`), +which is what keeps a second send, an autosave or a screenshot out of the way; the claim is renewed +by every checkpoint. `debugsession.not_uploaded` / `images_pending` read that progress. """ from collections.abc import Callable, Iterator @@ -161,16 +162,23 @@ def deliver( *, checkpoint: Callable[[], None], before_create: Callable[[], None] | None = None, + on_created: Callable[[], None] | None = None, ) -> str: - ( - """Send the session's issue, or finish sending it; returns its URL. `checkpoint` persists what - has happened so far and is called after every step (the caller's commit — this module does not - decide when a transaction ends). `before_create` runs right before the tracker is asked to open - a NEW issue — after the checks that can refuse it without asking — and may raise to stop it.""", - ) + """Send the session's issue, or finish sending it; returns its URL. + + `checkpoint` persists what has happened so far and is called after every step (the caller's + commit — this module does not decide when a transaction ends). `before_create` runs right before + the tracker is asked to open a NEW issue, after the checks that can refuse it without asking, + and may raise to stop it; `on_created` runs right after one was opened, in time to be committed + by the SAME checkpoint that first writes `issue_url` — anything the caller must not be able to + lose separately from the link belongs there. + """ state = debugsession.delivery(session) files = bundle.files(db, session) attachments = [f for f in files if f.name != "issue.md"] + # Reading the bundle and the tracker's settings opened a transaction; end it before the first + # call, so nothing holds a snapshot (and an idle connection) across minutes of tracker I/O. + checkpoint() with _client(db) as (client, repo): _private_repo(client, repo) if "number" not in state: @@ -197,6 +205,8 @@ def deliver( } session.issue_url = created["html_url"] debugsession.set_delivery(session, state) + if on_created is not None: + on_created() checkpoint() log.info( "issue created", @@ -223,7 +233,7 @@ def deliver( } debugsession.set_delivery(session, state) checkpoint() - shots = [n for n in state["links"] if n.startswith("screenshot-")] + shots = [n for n in state["links"] if n.startswith(debugsession.SCREENSHOT_PREFIX)] if shots and not state.get("images_shown"): images = "\n".join(f"![{n}]({state['links'][n]})" for n in sorted(shots)) body = f"{bundle.body_of(session)}\n\n---\n\n{images}" diff --git a/backend/app/routes/debug_sessions.py b/backend/app/routes/debug_sessions.py index 22b280b8..db909b6f 100644 --- a/backend/app/routes/debug_sessions.py +++ b/backend/app/routes/debug_sessions.py @@ -188,6 +188,15 @@ def export_session( # one, or an attempt refused before the tracker was asked, is not counted. The composer sends one at # a time; this is room for a bad afternoon, not a pace anything legitimate keeps up. _new_issues = RateLimiter(max_events=5, window_seconds=3600) +# Attempts on the tracker per hour, creating or finishing. Deliberately NOT the budget above: a +# half-sent issue must always be finishable, so the pace that guards creating one must never be what +# stands between a user and their own report. This one only bounds a loop — every attempt re-reads +# the whole bundle into memory and re-offers the pending file to the tracker, and a retry that is +# refused for good (the tracker's own limits) never leaves `pending`. No hand reaches it. +_sends = RateLimiter(max_events=60, window_seconds=3600) +# The composer's live preview, rendered BY the tracker: one authenticated call each, with our token. +# react-query debounces it to a handful a minute; this bounds what a loop can aim at the tracker. +_previews = RateLimiter(max_events=60, window_seconds=60) def _issues_rate_limited() -> HTTPException: @@ -216,7 +225,9 @@ def send_issue( raise refused(422, "issue_title_required") if _new_issues.exhausted(key): raise _issues_rate_limited() - debugsession.claim_send(session) + if not _sends.allow(key): + raise refused(429, "issue_rate_limited", limit=_sends.max_events) + token = debugsession.claim_send(session) db.commit() # the claim is what everything else sees; the lock ends here def count_new_issue() -> None: @@ -224,7 +235,13 @@ def send_issue( raise _issues_rate_limited() try: - gitea.deliver(db, session, checkpoint=db.commit, before_create=count_new_issue) + gitea.deliver( + db, + session, + checkpoint=db.commit, + before_create=count_new_issue, + on_created=lambda: _record_created(db, user, session), + ) except gitea.GiteaError as exc: # Keep what did happen: an issue created before an upload failed stays linked, and the # retry does only what is still missing. @@ -232,18 +249,17 @@ def send_issue( except HTTPException: raise # the pace, refused before the tracker was asked finally: - _end_send(db, user, session, new) + _end_send(db, session, token) return {"ok": True, "session": _view(db, session)} -def _end_send(db: Session, user: User, session: DebugSession, new: bool) -> None: - """Give the claim back, and record an issue this send opened — whatever else happened. A - failure here must not replace the failure that brought us: a dead transaction (the database - went away mid-send) is rolled back, and the claim then simply ages out.""" +def _end_send(db: Session, session: DebugSession, token: str) -> None: + """Give the claim back, whatever else happened. A failure here must not replace the failure that + brought us: a dead transaction (the database went away mid-send) is rolled back, and the claim + then simply ages out.""" try: debugsession.lock(db, session) - debugsession.release_send(session) - _record_created(db, user, session, new) + debugsession.release_send(session, token) db.commit() except Exception: # noqa: BLE001 — whatever went wrong here, the send's own failure is the news db.rollback() @@ -253,16 +269,19 @@ def _end_send(db: Session, user: User, session: DebugSession, new: bool) -> None ) -def _record_created(db: Session, user: User, session: DebugSession, new: bool) -> None: - if new and session.issue_url: - audit.record( - db, - user.id, - AuditAction.ISSUE_CREATE, - target_type="debug_session", - target_id=session.id, - summary=session.issue_url, - ) +def _record_created(db: Session, user: User, session: DebugSession) -> None: + """The trail for the one act that takes a user's log off this instance. Staged inside the send, + so the checkpoint that first writes `issue_url` commits BOTH — recorded later it could be lost + on its own (the process dies mid-send), and no retry could write it afterwards: the next send + sees the link and knows the issue is not new.""" + audit.record( + db, + user.id, + AuditAction.ISSUE_CREATE, + target_type="debug_session", + target_id=session.id, + summary=session.issue_url or "", + ) class PreviewIn(BaseModel): @@ -279,6 +298,8 @@ def preview_issue( """The body as the tracker will render it. Gitea sanitises its own output; the composer still shows it in a sandboxed frame, never in the app's own document.""" owned_or_404(db, DebugSession, sid, user, what="debug_session") + if not _previews.allow(str(user.id)): + raise refused(429, "preview_rate_limited") try: return {"ok": True, "html": gitea.preview(db, body.text)} except gitea.GiteaError as exc: diff --git a/backend/tests/test_debug_sessions.py b/backend/tests/test_debug_sessions.py index fac4eba3..27094b26 100644 --- a/backend/tests/test_debug_sessions.py +++ b/backend/tests/test_debug_sessions.py @@ -18,7 +18,7 @@ from datetime import datetime, timedelta, timezone import httpx import pytest from fastapi import BackgroundTasks -from sqlalchemy import select +from sqlalchemy import func, select from app import debugsession, logs from app.models import AuditLog, DebugScreenshot, DebugSession, LogEvent, User @@ -508,28 +508,83 @@ def test_a_half_sent_session_keeps_its_screenshots_a_finished_one_lets_them_go(d assert client.delete(f"/api/debug-sessions/{sid}/screenshots/{shot}").status_code == 200 -def test_a_send_under_way_holds_the_draft_and_the_screenshots_without_blocking_stop(db, recording): +def _age_the_claim(session, hours=1): + """The claim a send holds, as a request that died an hour ago left it.""" + state = debugsession.delivery(session) + stale = (datetime.now(timezone.utc) - timedelta(hours=hours)).isoformat() + session.draft = { + **(session.draft or {}), + "delivery": {**state, "sending": {**state["sending"], "at": stale}}, + } + + +def test_a_send_under_way_holds_the_draft_and_the_screenshots_without_blocking_stop( + gitea_fake, db, recording +): client, sid = recording + shot = _upload(client, sid, _PNG).json()["screenshot"]["id"] + _ready(client, sid) session = db.get(DebugSession, sid) debugsession.claim_send(session) db.commit() r = client.put(f"/api/debug-sessions/{sid}/draft", json={"title": "x", "body": "y"}) assert r.status_code == 409 and r.json()["detail"]["reason"] == "issue_sending" assert _upload(client, sid, _PNG).json()["detail"]["reason"] == "issue_sending" + # Nor removed: the send names the screenshots by their order, and this would renumber them + # under it — even before the issue exists, when nothing else would refuse it yet. + r = client.delete(f"/api/debug-sessions/{sid}/screenshots/{shot}") + assert r.status_code == 409 and r.json()["detail"]["reason"] == "issue_sending" r = client.post(f"/api/debug-sessions/{sid}/issue") assert r.status_code == 409 and r.json()["detail"]["reason"] == "issue_sending" + assert client.get(f"/api/debug-sessions/{sid}").json()["sending"] is True # Stop is NOT held: the recording is the user's to end while their report is going out. assert client.post(f"/api/debug-sessions/{sid}/stop").status_code == 200 - # A claim left behind by a request that died is taken over by the next send. - debugsession.set_delivery( - session, - {"sending": (datetime.now(timezone.utc) - timedelta(hours=1)).isoformat()}, - ) + # A claim left behind by a request that died is taken over by the next send — which then + # actually sends. Without the take-over the report could never leave, and nothing in the UI + # would say why. + _age_the_claim(session) db.commit() - assert ( - client.put(f"/api/debug-sessions/{sid}/draft", json={"title": "x", "body": "y"}).status_code - == 200 - ) + r = client.post(f"/api/debug-sessions/{sid}/issue") + assert r.status_code == 200 and r.json()["session"]["issue_url"] + assert client.get(f"/api/debug-sessions/{sid}").json()["sending"] is False + + +def test_a_slow_send_keeps_its_own_claim_alive_and_gives_back_only_that_one(db, recording): + """The lease is renewed by the progress the send reports: a send slower than the lease must not + expire while it is still working, and then overwrite whoever took its place.""" + client, sid = recording + session = db.get(DebugSession, sid) + token = debugsession.claim_send(session) + _age_the_claim(session) + assert debugsession.sending(session) is False + # One more step reported — proof of life, so the claim stands again. + debugsession.set_delivery(session, {"number": 7, "pending": ["server.log"], "links": {}}) + assert debugsession.sending(session) is True + assert debugsession.not_uploaded(session) == ["server.log"] + # The claim of a send that took over after ours aged out is not ours to hand back. + debugsession.release_send(session, "a-different-send") + assert debugsession.sending(session) is True + debugsession.release_send(session, token) + assert debugsession.sending(session) is False + + +def test_a_users_uploads_wait_for_each_other_across_their_sessions(db): + """The cap below is the user's across ALL their sessions, but a session's row lock only bounds + that session — two uploads to two sessions both read the total before either commits, and both + pass. The user's own lock is what makes the cap a cap instead of a guess.""" + from app.db import SessionLocal + + debugsession.lock_user(db, 4242) + other = SessionLocal() + try: + # A second request for the same user cannot get in to read the total while we hold it... + assert other.scalar(select(func.pg_try_advisory_xact_lock(4242))) is False + # ...and another user is not queued behind them for it. + assert other.scalar(select(func.pg_try_advisory_xact_lock(4243))) is True + finally: + other.rollback() + other.close() + db.rollback() def test_a_users_screenshots_are_capped_across_their_sessions(recording, monkeypatch): @@ -767,9 +822,11 @@ def gitea_fake(monkeypatch): monkeypatch.setattr(settings, "issues_gitea_url", "https://git.example/") monkeypatch.setattr(settings, "issues_gitea_repo", "o/r") monkeypatch.setattr(settings, "issues_gitea_token", "t0ken") - routes._new_issues.reset() + for limiter in (routes._new_issues, routes._sends, routes._previews): + limiter.reset() yield fake - routes._new_issues.reset() + for limiter in (routes._new_issues, routes._sends, routes._previews): + limiter.reset() def _ready(client, sid, title="Spinner stays"): @@ -837,6 +894,69 @@ def test_the_claim_is_held_through_every_step_of_a_send(gitea_fake, db, make_use assert held and all(held) # after the issue was created, and after every upload +def test_the_issues_trail_is_written_by_the_step_that_writes_the_link( + gitea_fake, db, make_user, as_user +): + """An issue whose link is committed without its trail can never get one: the next send reads + the link, knows the issue is not new, and records nothing. So the two go in one step.""" + from app.issues import gitea + from app.routes import debug_sessions as routes + + user = make_user() + client = as_user(user) + sid = _start(client)["id"] + _upload(client, sid, _PNG) + _ready(client, sid) + session = db.get(DebugSession, sid) + debugsession.claim_send(session) + db.commit() + seen: list[tuple[bool, int]] = [] + + def checkpoint() -> None: + db.commit() + rows = db.scalar( + select(func.count()).select_from(AuditLog).where(AuditLog.action == "issue_create") + ) + seen.append((session.issue_url is not None, rows or 0)) + + gitea.deliver( + db, + session, + checkpoint=checkpoint, + on_created=lambda: routes._record_created(db, user, session), + ) + # Every commit that can be seen with the link can be seen with the trail beside it. + assert seen and all(rows == 1 for linked, rows in seen if linked) + + +def test_a_send_that_cannot_give_its_claim_back_still_reports_its_own_failure( + gitea_fake, db, make_user, as_user, monkeypatch, caplog +): + """The rescue in `_end_send` is there so a dead transaction cannot replace the news: what the + user sees must be why the SEND failed, not why the tidying up did.""" + user = make_user() + client = as_user(user) + sid = _start(client)["id"] + _upload(client, sid, _PNG) + _ready(client, sid) + gitea_fake.fail_asset = "client.log" + locks = {"n": 0} + real_lock = debugsession.lock + + def flaky(db_session, session): + locks["n"] += 1 + if locks["n"] > 1: # the route's own lock works; the one in `_end_send` does not + raise RuntimeError("the database went away") + return real_lock(db_session, session) + + monkeypatch.setattr(debugsession, "lock", flaky) + with caplog.at_level(logging.WARNING): + r = client.post(f"/api/debug-sessions/{sid}/issue") + assert r.status_code == 422 and r.json()["detail"]["reason"] == "issues_unreachable" + events = [getattr(x, "event", "") for x in caplog.records] + assert "issues.claim.stuck" in events + + def test_a_public_repository_gets_nothing(gitea_fake, db, client, make_user): gitea_fake.private = False _signed_in(client, make_user) @@ -903,6 +1023,48 @@ def test_an_answer_that_is_not_the_api_is_refused_and_the_issue_stays_linked( assert [m for m, p in gitea_fake.calls if p == "/repos/o/r/issues"] == ["POST"] +def test_the_preview_has_a_pace_of_its_own(gitea_fake, db, make_user, as_user): + """It is one authenticated call to the operator's tracker each, with the operator's token.""" + from app.routes import debug_sessions as routes + + client = as_user(make_user()) + sid = _start(client)["id"] + monkeypatch_max = routes._previews.max_events + routes._previews.max_events = 2 + try: + for _ in range(2): + assert ( + client.post(f"/api/debug-sessions/{sid}/preview", json={"text": "x"}).status_code + == 200 + ) + r = client.post(f"/api/debug-sessions/{sid}/preview", json={"text": "x"}) + assert r.status_code == 429 and r.json()["detail"]["reason"] == "preview_rate_limited" + finally: + routes._previews.max_events = monkeypatch_max + + +def test_a_retry_that_never_finishes_is_still_paced(gitea_fake, db, make_user, as_user): + """The budget that guards OPENING an issue must never stand between a user and finishing their + own report — so the retry has a separate, far looser one, that only a loop reaches.""" + from app.routes import debug_sessions as routes + + client = as_user(make_user()) + sid = _start(client)["id"] + _upload(client, sid, _PNG) + _ready(client, sid) + was = routes._sends.max_events + routes._sends.max_events = 2 + try: + gitea_fake.fail_asset = "client.log" + assert client.post(f"/api/debug-sessions/{sid}/issue").status_code == 422 + gitea_fake.fail_asset = "client.log" + assert client.post(f"/api/debug-sessions/{sid}/issue").status_code == 422 + r = client.post(f"/api/debug-sessions/{sid}/issue") + assert r.status_code == 429 and r.json()["detail"]["reason"] == "issue_rate_limited" + finally: + routes._sends.max_events = was + + def test_a_redirecting_tracker_is_refused_not_a_crash(gitea_fake, db, make_user, as_user, caplog): gitea_fake.redirect = True admin = as_user(make_user(role="admin")) diff --git a/frontend/src/components/DebugRecorder.dom.test.tsx b/frontend/src/components/DebugRecorder.dom.test.tsx index f646ea72..e12ff690 100644 --- a/frontend/src/components/DebugRecorder.dom.test.tsx +++ b/frontend/src/components/DebugRecorder.dom.test.tsx @@ -25,7 +25,8 @@ vi.mock("react-i18next", async (importOriginal) => { }; }); -const { order, session, startDebugSession, stopDebugSession, hold } = vi.hoisted(() => { +const { order, session, startDebugSession, stopDebugSession, hold, past } = vi.hoisted(() => { + const past: { items: unknown[] } = { items: [] }; // A Stop held open until released, when a test needs it in flight. const hold = { stop: null as Promise | null }; const order: string[] = []; @@ -39,6 +40,7 @@ const { order, session, startDebugSession, stopDebugSession, hold } = vi.hoisted issue_url: null, pending_files: [], images_pending: false, + sending: false, screenshots: 0, draft: null, ...over, @@ -54,11 +56,19 @@ const { order, session, startDebugSession, stopDebugSession, hold } = vi.hoisted session: session({ id, ended_at: new Date().toISOString(), ended_reason: "stopped" }), }; }); - return { order, session, startDebugSession, stopDebugSession, hold }; + return { order, session, startDebugSession, stopDebugSession, hold, past }; }); vi.mock("../lib/api", async (importOriginal) => { const actual = await importOriginal(); - return { ...actual, api: { ...actual.api, startDebugSession, stopDebugSession } }; + return { + ...actual, + api: { + ...actual.api, + startDebugSession, + stopDebugSession, + debugSessions: () => Promise.resolve(past), + }, + }; }); vi.mock("../lib/diagnostics", async (importOriginal) => { const actual = await importOriginal(); @@ -71,7 +81,7 @@ vi.mock("../lib/diagnostics", async (importOriginal) => { }; }); -import DebugRecorder from "./DebugRecorder"; +import DebugRecorder, { RecorderSettings } from "./DebugRecorder"; import Modal from "./ui/Modal"; import Toaster from "./ui/Toaster"; import { @@ -80,7 +90,7 @@ import { setStepPrompt, useComposerSession, } from "../lib/debugRecorder"; -import { anyBlockingLayer } from "../lib/layerStack"; +import { anyBlockingLayer, anyBlockingLayerAbove, pushLayer, removeLayer } from "../lib/layerStack"; import { events } from "../lib/log"; beforeEach(() => { @@ -217,15 +227,22 @@ describe("the recorder pill", () => { expect(unhandled).toBe(false); }); - it("the step prompt owns the keyboard: the page's keys stay quiet and Tab stays in it", () => { + it("the step prompt quiets the page's keys and leaves the Tab to the dialog it sits in", () => { mount({ debug_session: session() }); expect(anyBlockingLayer()).toBe(false); + // The pill docks inside whatever dialog is open; stand one in for it, under the prompt. + const dialog = pushLayer(); fireEvent.click(within(pill()!).getByRole("button", { name: "recorder.pill.step" })); - // No dialog under it here: the prompt itself is what keeps `j`/`g h` off the page behind. + + // The prompt's field has the keyboard, so `j` and the `g` chords stay off the page behind... + expect(anyBlockingLayer()).toBe(true); + // ...while the dialog around it keeps its focus trap. The prompt cannot be that trap: its two + // controls are two of the dialog's many, and taking it left the rest of them untrapped, with + // Tab walking out into the page behind the scrim. + expect(anyBlockingLayerAbove(dialog)).toBe(false); + // And the quiet is the prompt's own doing, not the stand-in's: it outlasts the dialog. + removeLayer(dialog); expect(anyBlockingLayer()).toBe(true); - const mark = screen.getByRole("button", { name: "recorder.pill.mark" }); - mark.focus(); - expect(fireEvent.keyDown(mark, { key: "Tab" })).toBe(false); // trapped in the prompt }); it("the step prompt keeps what was typed when the pill moves to a dialog and back", () => { @@ -293,3 +310,72 @@ describe("the start dialog", () => { expect(screen.queryByText("recorder.start.title")).toBeNull(); }); }); + +describe("the past sessions a user goes back to check", () => { + function settings() { + const qc = new QueryClient({ defaultOptions: { queries: { retry: false } } }); + qc.setQueryData(qk.me(), { + id: 1, + is_demo: false, + preferences: {}, + diagnostics: { level: "INFO", until: null, retention_days: 7, session_minutes: 60 }, + debug_session: null, + }); + render( + + + , + ); + } + + it("does not call a half-delivered report sent", async () => { + // An issue that still owes its logs, or an edit that shows its screenshots, is not done. This + // list is where a user checks back, and a green "Sent" here is how one gets forgotten. + const over = { ended_at: new Date().toISOString(), ended_reason: "stopped" as const }; + past.items = [ + session({ + ...over, + id: "aaaaaaaaaaaaaaa1", + issue_url: "https://git.example/o/r/issues/7", + pending_files: ["client.log"], + }), + session({ + ...over, + id: "aaaaaaaaaaaaaaa2", + issue_url: "https://git.example/o/r/issues/8", + images_pending: true, + }), + session({ ...over, id: "aaaaaaaaaaaaaaa3", issue_url: "https://git.example/o/r/issues/9" }), + ]; + settings(); + + expect(await screen.findAllByText("recorder.history.partial")).toHaveLength(2); + expect(screen.getAllByText("recorder.history.sent")).toHaveLength(1); + }); + + it("says a send is still running rather than naming what it has got to so far", async () => { + // This is the list a user opens TO CHECK on a send. Its two honest-looking words are both + // wrong for a running one: "draft" before it has an issue, "half sent" after. + const over = { ended_at: new Date().toISOString(), ended_reason: "stopped" as const }; + past.items = [ + session({ + ...over, + id: "bbbbbbbbbbbbbbb1", + sending: true, + draft: { title: "t", body: "b", excluded: [], form: {} }, + }), + session({ + ...over, + id: "bbbbbbbbbbbbbbb2", + sending: true, + issue_url: "https://git.example/o/r/issues/7", + pending_files: ["client.log"], + }), + ]; + settings(); + + expect(await screen.findAllByText("recorder.history.sending")).toHaveLength(2); + expect(screen.queryByText("recorder.history.draft")).toBeNull(); + expect(screen.queryByText("recorder.history.partial")).toBeNull(); + }); +}); diff --git a/frontend/src/components/DebugRecorder.tsx b/frontend/src/components/DebugRecorder.tsx index 9d47ef18..e28f5db2 100644 --- a/frontend/src/components/DebugRecorder.tsx +++ b/frontend/src/components/DebugRecorder.tsx @@ -3,7 +3,7 @@ import { useTranslation } from "react-i18next"; import { useQuery, useQueryClient } from "@tanstack/react-query"; import { Camera, Flag, Square } from "lucide-react"; -import { api, type DebugSessionLevel } from "../lib/api"; +import { api, type DebugSession, type DebugSessionLevel } from "../lib/api"; import { canCaptureTab } from "../lib/captureTab"; import { closeRecorderStart, @@ -23,7 +23,6 @@ import { qk } from "../lib/queryKeys"; import { dockUnderToasts } from "../lib/toastDock"; import { useMe } from "../lib/useMe"; import { Button } from "./ui/Button"; -import { trapTab } from "./ui/dialogFocus"; import Modal from "./ui/Modal"; import { LoadingState, StateMessage } from "./ui/QueryState"; import SegmentedToggle from "./ui/SegmentedToggle"; @@ -287,15 +286,16 @@ function SettingsElapsed({ since }: { since: string }) { /** "What did you just do?" — optional; Enter marks the step, Escape leaves it unmarked. */ function StepPrompt({ id, onDone }: { id: string; onDone: (label: string | null) => void }) { const { t } = useTranslation(); - const box = useRef(null); const n = useStepCount(id) + 1; const label = useStepPrompt() ?? ""; // A layer of its own, so Escape closes only the prompt — not the dialog or the player the pill may // be sitting in: the topmost layer takes Escape (lib/layerStack), and this one opened last. It - // OWNS the keyboard while it is up (an autofocused field does), so the page's own keys stay quiet - // — and it therefore traps Tab itself, between its field and its button: a dialog under it defers - // its trap to the layer above, and without one of our own Tab walked out into the page behind. - useLayer(true, () => onDone(null)); + // OWNS the keyboard while it is up (an autofocused field does), so the page's own keys stay quiet. + // It does NOT take the Tab, though: it is a field inside whatever dialog the pill is docked in, + // not a dialog of its own, and the trap belongs to that one — it spans the prompt as it spans the + // rest of the toast host. Claiming it here switched the dialog's off and left every control + // OUTSIDE this little box untrapped (deep-review round 3). + useLayer(true, () => onDone(null), { trapsTab: false }); const onKeyDown = (e: ReactKeyboardEvent) => { if (e.key === "Enter") { e.preventDefault(); @@ -303,13 +303,7 @@ function StepPrompt({ id, onDone }: { id: string; onDone: (label: string | null) } }; return ( -
{ - if (e.key === "Tab" && box.current) trapTab(e.nativeEvent, box.current); - }} - className="absolute bottom-full left-0 mb-2 w-80 max-w-[calc(100vw-2rem)] glass rounded-xl p-3 flex flex-col gap-2 shadow-lg" - > +
@@ -332,6 +326,27 @@ function StepPrompt({ id, onDone }: { id: string; onDone: (label: string | null) ); } +/** What a past session reads as. An issue that still owes the tracker its attachments, or an edit + * that shows its screenshots, is NOT "Sent": this list is where a user goes back to check, and a + * green chip there is how a half-delivered report gets forgotten. */ +function historyState(s: DebugSession): string { + // A send under way outranks every other word here: this is the list a user comes back to in + // order to check on one, and both "draft" (no issue yet) and "half sent" (the steps it has + // already committed) would describe a send that STOPPED. + if (s.sending) return "recorder.history.sending"; + if (!s.issue_url) return s.draft ? "recorder.history.draft" : "recorder.history.unwritten"; + return s.pending_files.length > 0 || s.images_pending + ? "recorder.history.partial" + : "recorder.history.sent"; +} + +const chipTone = (s: DebugSession): string => { + const state = historyState(s); + if (state === "recorder.history.sent") return "text-success"; + if (state === "recorder.history.sending") return "text-accent"; + return state === "recorder.history.partial" ? "text-warn" : ""; +}; + /** The last sessions, newest first — reopening one opens its composer, with what was written. */ function SessionHistory() { const { t, i18n } = useTranslation(); @@ -362,14 +377,8 @@ function SessionHistory() { {s.draft?.title || t(`settings.diagnostics.levels.${s.level}`)} - - {t( - s.issue_url - ? "recorder.history.sent" - : s.draft - ? "recorder.history.draft" - : "recorder.history.unwritten", - )} + + {t(historyState(s))} + {/* Nothing to retry while it is still going — the button would only be refused. */} + {!sending && ( + + )}
)} {sent && (
{t("recorder.composer.sent")} {target && !sent && ( - )} diff --git a/frontend/src/i18n/locales/en/errors.json b/frontend/src/i18n/locales/en/errors.json index d57f8177..c7ccd936 100644 --- a/frontend/src/i18n/locales/en/errors.json +++ b/frontend/src/i18n/locales/en/errors.json @@ -226,6 +226,8 @@ "debug_session_rate_limited": "You can start at most {{limit}} new recordings an hour. Try again later.", "export_rate_limited": "Too many downloads in a row. Wait a minute.", "screenshot_storage_full": "You've reached the {{limit}} MB screenshot allowance. Remove images from older sessions, or wait until they expire.", - "issue_sending": "The report is being sent right now. Wait for it to finish." + "issue_sending": "The report is being sent right now. Wait for it to finish.", + "issue_rate_limited": "At most {{limit}} send attempts an hour. Try again later.", + "preview_rate_limited": "Too many previews in a row. Wait a minute." } } diff --git a/frontend/src/i18n/locales/en/recorder.json b/frontend/src/i18n/locales/en/recorder.json index 00681146..9f214303 100644 --- a/frontend/src/i18n/locales/en/recorder.json +++ b/frontend/src/i18n/locales/en/recorder.json @@ -109,6 +109,8 @@ "shotsFailed": "Couldn't load the screenshots", "removeShotFailed": "Couldn't remove the screenshot", "partialImages": "The issue was created, but its text does not show the screenshots yet.", + "retryImages": "Show them in the issue", + "sendingNow": "The report is being sent…", "exportFailed": "Couldn't build the zip" }, "issue": { @@ -128,6 +130,8 @@ "empty": "No session recorded yet.", "open": "Open", "sent": "Sent", + "partial": "Half sent", + "sending": "Sending", "draft": "Draft", "unwritten": "Not written yet", "failed": "Couldn't load the sessions" diff --git a/frontend/src/i18n/locales/hu/errors.json b/frontend/src/i18n/locales/hu/errors.json index fa6de540..d571b979 100644 --- a/frontend/src/i18n/locales/hu/errors.json +++ b/frontend/src/i18n/locales/hu/errors.json @@ -226,6 +226,8 @@ "debug_session_rate_limited": "Óránként legfeljebb {{limit}} új rögzítést indíthatsz. Próbáld újra később.", "export_rate_limited": "Túl sok letöltés egymás után. Várj egy percet.", "screenshot_storage_full": "Elérted a képernyőképek {{limit}} MB-os keretét. Törölj régebbi munkamenetekből képeket, vagy várd meg, amíg lejárnak.", - "issue_sending": "A hibajegy küldése épp folyamatban van. Várd meg, amíg befejeződik." + "issue_sending": "A hibajegy küldése épp folyamatban van. Várd meg, amíg befejeződik.", + "issue_rate_limited": "Óránként legfeljebb {{limit}} küldési kísérlet. Próbáld újra később.", + "preview_rate_limited": "Túl sok előnézet egymás után. Várj egy percet." } } diff --git a/frontend/src/i18n/locales/hu/recorder.json b/frontend/src/i18n/locales/hu/recorder.json index fba6689d..6783c30f 100644 --- a/frontend/src/i18n/locales/hu/recorder.json +++ b/frontend/src/i18n/locales/hu/recorder.json @@ -109,6 +109,8 @@ "shotsFailed": "Nem sikerült betölteni a képernyőképeket", "removeShotFailed": "Nem sikerült törölni a képernyőképet", "partialImages": "A hibajegy létrejött, de a képernyőképek még nem látszanak a szövegében.", + "retryImages": "Megjelenítés a hibajegyben", + "sendingNow": "A hibajegy küldése folyamatban…", "exportFailed": "Nem sikerült elkészíteni a zipet" }, "issue": { @@ -128,6 +130,8 @@ "empty": "Még nincs rögzített munkamenet.", "open": "Megnyitás", "sent": "Elküldve", + "partial": "Félig elküldve", + "sending": "Küldés alatt", "draft": "Vázlat", "unwritten": "Megírásra vár", "failed": "Nem sikerült betölteni a munkameneteket" diff --git a/frontend/src/lib/api/core.test.ts b/frontend/src/lib/api/core.test.ts index 83b922a0..c71b2b01 100644 --- a/frontend/src/lib/api/core.test.ts +++ b/frontend/src/lib/api/core.test.ts @@ -216,6 +216,40 @@ describe("req — a structured refusal reads in the user's language", () => { ); }); +describe("req — a file, not a parsed answer", () => { + it("hands back the response itself so the caller can read it as a blob", async () => { + // A zip. Without the `raw` branch this falls into `r.json()`, throws on the first byte, and the + // download dies as an opaque failure — which no lane would see, because the only consumer test + // mocks the api method wholesale. + const zip = new Response(new Blob(["PK"]), { + status: 200, + headers: { + "content-type": "application/zip", + "content-disposition": 'attachment; filename="siftlode-debug-260919.zip"', + }, + }); + vi.spyOn(globalThis, "fetch").mockResolvedValue(zip); + + const r = await req("/api/debug-sessions/abc/export", {}, { raw: true }); + + expect(r).toBe(zip); + expect(r.headers.get("content-disposition")).toContain("siftlode-debug-260919.zip"); + expect(await r.blob()).toBeInstanceOf(Blob); // the body is still there to be read + }); + + it("still refuses before the body is touched, so a refusal is shown and not saved", async () => { + vi.spyOn(globalThis, "fetch").mockResolvedValue( + new Response(JSON.stringify({ detail: { code: "refused", reason: "export_rate_limited" } }), { + status: 429, + headers: { "content-type": "application/json" }, + }), + ); + await expect( + req("/api/debug-sessions/abc/export", {}, { raw: true, quiet: true }), + ).rejects.toMatchObject({ status: 429, detail: "errors.refused.export_rate_limited" }); + }); +}); + describe("req — what it has already put in front of the user", () => { // A caller that toasts a failed action on top of the dialog `req` already opened reported one fault // twice. `alreadyReported` is how `notifyActionError` tells the two apart. diff --git a/frontend/src/lib/api/core.ts b/frontend/src/lib/api/core.ts index f090420e..5d8f69d3 100644 --- a/frontend/src/lib/api/core.ts +++ b/frontend/src/lib/api/core.ts @@ -360,9 +360,11 @@ export async function req( reportFailure(err, !!cfg.quiet); throw err; } - // A file (`cfg.raw`): the answer itself, for the caller to read as a blob. A download that went - // out as a plain `` instead would carry no refusal the app could show — the browser - // would simply save the error. + // A file (`cfg.raw`): the answer itself, for the caller to read as a blob. For a SMALL generated + // file whose endpoint can refuse (a report, an export): sent as a plain `` it would + // carry no refusal the app could show — the browser would simply save the error page. A large + // stored file stays a plain navigation (the download centre, the Plex original), so the browser + // streams it to disk instead of buffering gigabytes into the tab. if (cfg.raw) return r as T; // The single audited cast in this layer: the server contract is asserted here, once, so // every typed api.* method (T comes from its declared return type) is checked from here up. diff --git a/frontend/src/lib/api/diagnostics.dom.test.ts b/frontend/src/lib/api/diagnostics.dom.test.ts new file mode 100644 index 00000000..68ba8010 --- /dev/null +++ b/frontend/src/lib/api/diagnostics.dom.test.ts @@ -0,0 +1,57 @@ +// @vitest-environment jsdom +// +// The zip export is the one api method that reads a RAW response instead of a parsed answer, so it +// carries contract of its own: the bytes, and the name the server picked. The composer's DOM test +// mocks this method wholesale, so without this the blob and the filename parse ship untested — and +// a broken header regex would silently save every report under the fallback name. +import { describe, it, expect, vi, beforeEach } from "vitest"; + +vi.mock("../errorDialog", () => ({ reportError: vi.fn() })); +vi.mock("../notifications", () => ({ + notify: vi.fn(() => 1), + remove: vi.fn(), + getActiveToasts: vi.fn((): { id: number }[] => []), +})); +vi.mock("../../i18n", () => ({ default: { t: (k: string) => k, language: "en" } })); +vi.mock("../activeAccount", () => ({ + activeAccountId: () => null, + ACTIVE_ACCOUNT_KEY: "k", + pinActiveAccount: () => {}, +})); + +import { debugSessionsApi } from "./diagnostics"; + +const zip = (disposition?: string) => + new Response("PK", { + status: 200, + headers: { + "content-type": "application/zip", + ...(disposition ? { "content-disposition": disposition } : {}), + }, + }); + +beforeEach(() => vi.restoreAllMocks()); + +describe("debugExport", () => { + it("hands back the bytes and the name the server chose", async () => { + vi.spyOn(globalThis, "fetch").mockResolvedValue( + zip('attachment; filename="siftlode-debug-20260919-1402-abc123.zip"'), + ); + + const out = await debugSessionsApi.debugExport("0123456789abcdef"); + + expect(out.name).toBe("siftlode-debug-20260919-1402-abc123.zip"); + // The body is read as bytes, not parsed: `r.json()` on a zip would throw on its first one. + expect(out.blob).toBeInstanceOf(Blob); + expect(out.blob.size).toBe(4); + }); + + it("names the file itself when the answer does not", async () => { + // A proxy that strips the header must not leave the browser saving something called "export". + vi.spyOn(globalThis, "fetch").mockResolvedValue(zip()); + + const out = await debugSessionsApi.debugExport("0123456789abcdef"); + + expect(out.name).toBe("siftlode-debug-0123456789abcdef.zip"); + }); +}); diff --git a/frontend/src/lib/api/diagnostics.ts b/frontend/src/lib/api/diagnostics.ts index 92197581..489c62a3 100644 --- a/frontend/src/lib/api/diagnostics.ts +++ b/frontend/src/lib/api/diagnostics.ts @@ -57,6 +57,9 @@ export interface DebugSession { pending_files: string[]; /** Whether the issue's body has still to be edited to show the screenshots it already carries. */ images_pending: boolean; + /** Whether a send is under way RIGHT NOW. Every step of a send commits its progress, so without + * this a reload or a second tab reads a running send as a failed one. */ + sending: boolean; screenshots: number; /** The issue being written from it (the composer saves as it goes); null until it first does. */ draft: DebugDraft | null; diff --git a/frontend/src/lib/diagnostics.session.dom.test.ts b/frontend/src/lib/diagnostics.session.dom.test.ts index 3036a4d1..0cb2ec0a 100644 --- a/frontend/src/lib/diagnostics.session.dom.test.ts +++ b/frontend/src/lib/diagnostics.session.dom.test.ts @@ -39,6 +39,7 @@ const recording = (level: "DEBUG" | "TRACE", minutes = 60, over = {}) => ({ issue_url: null, pending_files: [], images_pending: false, + sending: false, screenshots: 0, draft: null, ...over, diff --git a/frontend/src/lib/layerStack.dom.test.ts b/frontend/src/lib/layerStack.dom.test.ts index 3ec3e5ba..05014f99 100644 --- a/frontend/src/lib/layerStack.dom.test.ts +++ b/frontend/src/lib/layerStack.dom.test.ts @@ -12,6 +12,8 @@ import { pushLayer, removeLayer, anyLayerAbove, + anyBlockingLayer, + anyBlockingLayerAbove, isTopmost, markEscapeHandled, escapeHandled, @@ -77,3 +79,37 @@ describe("layerStack Escape coordination (UX-8)", () => { expect(escapeHandled(second)).toBe(false); }); }); + +// A layer answers TWO questions, and round 3 found they are not the same one: "do the page's own +// keys stay quiet?" and "do I take the Tab trap from the dialog below?". The recorder's step prompt +// is the case that separates them — a field inside whatever dialog the pill is docked in, so it +// owns the keyboard but its two controls are no trap for that dialog's many. +describe("owning the keyboard and owning the Tab trap", () => { + it("an inline prompt silences the page and still leaves the dialog its trap", () => { + const dialog = open(); + const prompt = pushLayer({ trapsTab: false }); + opened.push(prompt); + + expect(anyBlockingLayer()).toBe(true); // `j`, `g h` and the rest stay off the page behind + expect(anyBlockingLayerAbove(dialog)).toBe(false); // ...and the dialog keeps trapping Tab + }); + + it("a real dialog above does take the trap from the one below", () => { + const dialog = open(); + const above = open(); + + expect(anyBlockingLayerAbove(dialog)).toBe(true); + expect(anyBlockingLayerAbove(above)).toBe(false); + }); + + it("a passive layer takes neither: a hover card must silence nothing", () => { + const card = pushLayer({ passive: true }); + opened.push(card); + expect(anyBlockingLayer()).toBe(false); // `j` still moves the list under a resting pointer + + const dialog = open(); + const over = pushLayer({ passive: true }); + opened.push(over); + expect(anyBlockingLayerAbove(dialog)).toBe(false); // ...and it is no trap either + }); +}); diff --git a/frontend/src/lib/layerStack.ts b/frontend/src/lib/layerStack.ts index 573a61fa..aaea9a45 100644 --- a/frontend/src/lib/layerStack.ts +++ b/frontend/src/lib/layerStack.ts @@ -20,17 +20,25 @@ let nextId = 1; // stack for Escape ordering like any other layer, but `anyBlockingLayer` looks past them — otherwise // a tooltip that happens to be open under a resting mouse pointer would silence the page's `j`/`k`. const passive = new Set(); +// Layers that DO own the keyboard but leave the Tab trap to the dialog below: a small inline prompt +// inside one (the recorder's step field). It must silence the page's keys — an autofocused field +// does — yet it is not a dialog of its own, so claiming the trap left the dialog around it with +// none, and Tab from anything outside the prompt walked into the page behind the scrim. The two +// questions are separate; a layer answers each for itself (deep-review round 3). +const defers = new Set(); -export function pushLayer(opts?: { passive?: boolean }): number { +export function pushLayer(opts?: { passive?: boolean; trapsTab?: boolean }): number { const id = nextId++; stack.push(id); if (opts?.passive) passive.add(id); + if (opts?.trapsTab === false) defers.add(id); return id; } export function removeLayer(id: number): void { stack = stack.filter((x) => x !== id); passive.delete(id); + defers.delete(id); } /** True while a layer that owns the keyboard is open — a modal, a player, a click-opened menu. The @@ -51,12 +59,13 @@ export function anyLayerAbove(id: number): boolean { return i !== -1 && i < stack.length - 1; } -/** True when a layer that owns the keyboard sits above `id` — the predicate for a dialog's Tab trap. - * A tooltip or hover card open under a resting pointer takes Escape first, but not the Tab: deferring - * the trap to it let Shift+Tab walk out of the dialog into the page behind its scrim. */ +/** True when a layer that TRAPS THE TAB sits above `id` — the predicate for a dialog's Tab trap. A + * tooltip open under a resting pointer (passive) and an inline prompt inside the dialog itself + * (`trapsTab: false`) both take Escape first but neither takes the Tab: deferring the trap to one + * of those left Shift+Tab walking out of the dialog into the page behind its scrim. */ export function anyBlockingLayerAbove(id: number): boolean { const i = stack.indexOf(id); - return i !== -1 && stack.slice(i + 1).some((x) => !passive.has(x)); + return i !== -1 && stack.slice(i + 1).some((x) => !passive.has(x) && !defers.has(x)); } // UX-8 — a layer that ACTS on an Escape stamps the event here; a lower layer with its own key handler @@ -83,17 +92,20 @@ export function escapeHandled(e: KeyboardEvent): boolean { export function useLayer( active: boolean, onEscape?: () => void, - opts?: { passive?: boolean }, + opts?: { passive?: boolean; trapsTab?: boolean }, ): { isTopmost: () => boolean; anyAbove: () => boolean; blockingAbove: () => boolean } { const idRef = useRef(null); const escRef = useRef(onEscape); escRef.current = onEscape; - const passiveRef = useRef(!!opts?.passive); - passiveRef.current = !!opts?.passive; + const optsRef = useRef(opts); + optsRef.current = opts; useEffect(() => { if (!active) return; - const id = pushLayer({ passive: passiveRef.current }); + const id = pushLayer({ + passive: !!optsRef.current?.passive, + trapsTab: optsRef.current?.trapsTab, + }); idRef.current = id; let onKey: ((e: KeyboardEvent) => void) | undefined; if (escRef.current) {