From 3f983e6d765400471d4a59a13213c5433b6b016b Mon Sep 17 00:00:00 2001 From: npeter83 Date: Sun, 20 Sep 2026 00:51:12 +0200 Subject: [PATCH] =?UTF-8?q?fix(diagnostics):=20deep-review=20round=202=20?= =?UTF-8?q?=E2=80=94=20a=20send=20that=20claims,=20not=20one=20that=20bloc?= =?UTF-8?q?ks?= MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit The send no longer holds the session's row lock while it talks to the tracker: it claims the session, gives the lock back, and checkpoints each step. A Stop is never held behind a send, while a draft, a screenshot or a second send are refused at once instead of queueing. The claim ages out, so a request that died mid-send does not strand the session. What the composer says is what happened: an attachment that went is no longer named as one that did not (the missing body edit is its own line), and the issue's screenshot count comes from the server when the list cannot be read, so a failed read no longer blocks saving the draft. The zip is fetched as a request, so its pace is a message and not a saved error page. Screenshots of a finished report can be removed again; only one still being sent is held. A recording's log elevation can no longer outlive it. The step prompt keeps the keyboard (the page's keys stay quiet) and traps Tab itself. An image pasted into a dialog opened over the composer stays that dialog's. --- backend/app/debugsession.py | 87 +++++++++++++++---- backend/app/issues/gitea.py | 46 ++++++---- backend/app/routes/debug_sessions.py | 56 ++++++++---- backend/tests/test_debug_sessions.py | 69 ++++++++++++--- backend/tests/test_route_conventions.py | 2 + .../src/components/DebugRecorder.dom.test.tsx | 13 +++ frontend/src/components/DebugRecorder.tsx | 20 +++-- .../src/components/IssueComposer.dom.test.tsx | 59 ++++++++++--- frontend/src/components/IssueComposer.tsx | 71 ++++++++------- .../src/components/ui/Toaster.dom.test.tsx | 14 ++- frontend/src/i18n/locales/en/errors.json | 3 +- frontend/src/i18n/locales/en/recorder.json | 4 +- frontend/src/i18n/locales/hu/errors.json | 3 +- frontend/src/i18n/locales/hu/recorder.json | 4 +- frontend/src/lib/api/core.ts | 6 ++ frontend/src/lib/api/diagnostics.ts | 15 ++-- .../src/lib/diagnostics.session.dom.test.ts | 1 + 17 files changed, 347 insertions(+), 126 deletions(-) diff --git a/backend/app/debugsession.py b/backend/app/debugsession.py index 3f114733..829fcb96 100644 --- a/backend/app/debugsession.py +++ b/backend/app/debugsession.py @@ -132,11 +132,17 @@ def from_header(db: Session, user: User, value: str | None) -> DebugSession | No def level_for(user: User, session: DebugSession | None) -> tuple[str, datetime | None]: """The diagnostics level (and its expiry) a request is logged at: the user's own, raised to the - session's while one is named — whichever records more.""" + session's while one is named — whichever records more. + + A named session also caps the expiry at its own: this pair is what the process tracks under the + session (app/logs.py), and an expiry outliving the recording would hold the root logger down + after it ended.""" own = diagnostics.effective_level(user) until = user.diagnostics_until if own != diagnostics.BASE else None - if session is None or _DETAIL[session.level] <= _DETAIL[own]: + if session is None: return own, until + if _DETAIL[session.level] <= _DETAIL[own]: + return own, min(until, session.until) if until else session.until return session.level, session.until @@ -163,14 +169,56 @@ def delivery(session: DebugSession) -> dict: return dict((session.draft or {}).get("delivery") or {}) -def unfinished(session: DebugSession) -> list[str]: - """What a send that stopped half-way still owes its issue: the files not yet uploaded, and the - screenshots uploaded but not yet shown in its body (the edit that shows them failed).""" +def not_uploaded(session: DebugSession) -> list[str]: + """The files a send that stopped half-way has still to upload — by name, as the issue carries + them. What the composer names to the user.""" + return list(delivery(session).get("pending") or []) + + +def images_pending(session: DebugSession) -> bool: + """Whether the issue's body still has to be edited to show the screenshots it already carries + (every upload went, the edit that shows them did not).""" state = delivery(session) - missing = list(state.get("pending") or []) - if not state.get("images_shown"): - missing += [n for n in state.get("links") or {} if n.startswith("screenshot-")] - return missing + shown = state.get("images_shown") + return not shown and any(n.startswith("screenshot-") for n in state.get("links") or {}) + + +def unfinished(session: DebugSession) -> bool: + """Whether a send owes the issue anything — the retry's question.""" + return bool(not_uploaded(session)) or images_pending(session) + + +# 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. +SEND_CLAIM_SECONDS = 600 + + +def sending(session: DebugSession) -> bool: + """Whether a send is under way right now (a live claim).""" + since = delivery(session).get("sending") + if not since: + return False + started = datetime.fromisoformat(since) + return (datetime.now(timezone.utc) - started).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 release_send(session: DebugSession) -> None: + state = {k: v for k, v in delivery(session).items() if k != "sending"} + set_delivery(session, state) + + +def set_delivery(session: DebugSession, state: dict) -> None: + """Write a delivery's progress (app/issues/gitea.py keeps it; this module reads it).""" + # A new dict, so the JSONB column sees the change. + session.draft = {**(session.draft or {}), "delivery": state} def serialize(session: DebugSession | None, screenshots: int = 0) -> dict | None: @@ -184,8 +232,10 @@ def serialize(session: DebugSession | None, screenshots: int = 0) -> dict | None "ended_at": session.ended_at.isoformat() if session.ended_at else None, "ended_reason": session.ended_reason, "issue_url": session.issue_url, - # A send that stopped half-way: what it still owes. - "pending_files": unfinished(session), + # 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. + "pending_files": not_uploaded(session), + "images_pending": images_pending(session), "screenshots": screenshots, "draft": session.draft, } @@ -253,9 +303,10 @@ def add_screenshot(db: Session, session: DebugSession, data: bytes) -> DebugScre mime = sniff(data) if mime is None: raise ScreenshotRefused("not_an_image") - # Under the lock: a send in flight is waited for (and then refuses this), and two uploads at - # once cannot both pass the count. + # Under the lock: two uploads at once cannot both pass the count. lock(db, session) + if sending(session): + raise ScreenshotRefused("sending") if session.issue_url is not None: raise ScreenshotRefused("already_sent") if screenshot_count(db, session) >= SCREENSHOTS_MAX: @@ -275,10 +326,14 @@ def add_screenshot(db: Session, session: DebugSession, data: bytes) -> DebugScre def remove_screenshot(db: Session, session: DebugSession, shot: DebugScreenshot) -> None: - """Delete one screenshot, under the lock. Refused once the issue was sent: it carries this one - already, and a delivery names the screenshots by their order. The caller commits.""" + """Delete one screenshot, under the lock. Refused while a send is under way or unfinished: a + delivery names the screenshots by their order, and removing one would renumber what it still + has to send. Once the issue has everything, this only drops OUR copy — the issue keeps its + own. The caller commits.""" lock(db, session) - if session.issue_url is not None: + if sending(session): + raise ScreenshotRefused("sending") + if unfinished(session): raise ScreenshotRefused("already_sent") db.delete(shot) db.flush() diff --git a/backend/app/issues/gitea.py b/backend/app/issues/gitea.py index 62de2c62..2343f0fb 100644 --- a/backend/app/issues/gitea.py +++ b/backend/app/issues/gitea.py @@ -19,8 +19,10 @@ composer's own preview shows our copies, never these links. Delivery keeps its progress in the session's draft (`delivery`: the issue number, the files still to 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 flushes; the caller holds the session's lock throughout (`debugsession.lock`) and commits, -also when a step fails, so what did happen is kept. `debugsession.unfinished` reads that progress. +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. """ from collections.abc import Callable, Iterator @@ -110,8 +112,12 @@ def _call(client: httpx.Client, method: str, path: str, **kwargs) -> httpx.Respo def _json(r: httpx.Response, *keys: str) -> dict[str, Any]: - """The answer as the API's JSON, holding `keys` — anything else is refused, not a crash: a - crash after the issue was created would lose the record that it was.""" + """The answer as the API's JSON, holding `keys` — anything else is refused, not a crash, so a + step after the issue was created cannot take down the request that holds its record. + + The CREATE call is the one place this cannot keep that record: its answer IS the record. An + answer it cannot read there leaves the session unsent, and a retry opens another issue — the + price of not being able to tell a refused create from an unreadable one.""" try: data = r.json() # silent-ok: not JSON is one of the answers refused (and logged) just below. @@ -149,17 +155,19 @@ def preview(db: Session, text: str) -> str: return r.text -def _save(session: DebugSession, state: dict) -> None: - # A new dict, so the JSONB column sees the change. - session.draft = {**(session.draft or {}), "delivery": state} - - def deliver( - db: Session, session: DebugSession, *, before_create: Callable[[], None] | None = None + db: Session, + session: DebugSession, + *, + checkpoint: Callable[[], None], + before_create: Callable[[], None] | None = None, ) -> str: - """Send the session's issue, or finish sending it; returns its URL. Flushes after every step. - `before_create` runs right before a NEW issue is opened — after every check that could refuse - it — and may raise to stop it (the caller's pace on new issues).""" + ( + """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.""", + ) state = debugsession.delivery(session) files = bundle.files(db, session) attachments = [f for f in files if f.name != "issue.md"] @@ -184,8 +192,8 @@ def deliver( "links": {}, } session.issue_url = created["html_url"] - _save(session, state) - db.flush() + debugsession.set_delivery(session, state) + checkpoint() log.info( "issue created", extra=logs.event("issues.gitea.created", number=created["number"]), @@ -209,13 +217,13 @@ def deliver( "pending": [n for n in state["pending"] if n != item.name], "links": {**state["links"], item.name: asset["browser_download_url"]}, } - _save(session, state) - db.flush() + debugsession.set_delivery(session, state) + checkpoint() shots = [n for n in state["links"] if n.startswith("screenshot-")] 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}" _call(client, "PATCH", f"/repos/{repo}/issues/{number}", json={"body": body}) - _save(session, {**state, "images_shown": True}) - db.flush() + debugsession.set_delivery(session, {**state, "images_shown": True}) + checkpoint() return session.issue_url or "" diff --git a/backend/app/routes/debug_sessions.py b/backend/app/routes/debug_sessions.py index f6c7bc46..22b280b8 100644 --- a/backend/app/routes/debug_sessions.py +++ b/backend/app/routes/debug_sessions.py @@ -147,9 +147,11 @@ def save_draft( session = owned_or_404(db, DebugSession, sid, user, what="debug_session") if len(json.dumps(body.form, ensure_ascii=False).encode()) > FORM_MAX_BYTES: raise HTTPException(status_code=413, detail={"code": "too_large"}) - # Under the lock: a save that arrives while the issue is being sent waits for the send, and then - # sees it — the draft of a sent issue cannot change after it. + # Under the lock, and refused while a send is under way: the issue is being built from the draft + # as it was saved, and this would change it under the send. debugsession.lock(db, session) + if debugsession.sending(session): + raise refused(409, "issue_sending") if session.issue_url is not None: # Sent: the issue is on the tracker now, and a draft edited after it would say otherwise. raise refused(409, "issue_already_sent") @@ -181,9 +183,10 @@ def export_session( # --- the issue tracker (S4) ----------------------------------------------------------------- -# Issues a user may OPEN per hour — finishing a half-sent one, or an attempt refused before the issue -# was opened, is not counted. The composer sends one at a time; this is room for a bad afternoon, not -# a pace anything legitimate keeps up. +# Issues a user may open per hour — counted once the tracker is ASKED to open one (an answer that +# goes missing may still have created it, so an attempt cannot be given back); finishing a half-sent +# 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) @@ -198,10 +201,12 @@ def send_issue( """Send the session's issue to the tracker — from the SAVED draft, as the export is built — or finish a send that stopped half-way (the issue exists; only what is missing is sent).""" session = owned_or_404(db, DebugSession, sid, user, what="debug_session") - # Held to the end: a second send (another tab, a retry after a timeout) waits for this one and - # then finds the issue it opened, instead of opening another; an autosave or a screenshot waits - # too, and then finds the issue sent. + # The session is CLAIMED for the send rather than locked through it: the tracker is slow (a + # dozen calls, 20 s each), and a row lock held across them would queue this user's Stop, their + # autosave and their screenshots behind it. They see the claim instead and are refused at once. debugsession.lock(db, session) + if debugsession.sending(session): + raise refused(409, "issue_sending") new = session.issue_url is None if not new and not debugsession.unfinished(session): return {"ok": True, "session": _view(db, session)} @@ -211,30 +216,41 @@ def send_issue( raise refused(422, "issue_title_required") if _new_issues.exhausted(key): raise _issues_rate_limited() + debugsession.claim_send(session) + db.commit() # the claim is what everything else sees; the lock ends here def count_new_issue() -> None: if not _new_issues.allow(key): raise _issues_rate_limited() try: - gitea.deliver(db, session, before_create=count_new_issue) + gitea.deliver(db, session, checkpoint=db.commit, before_create=count_new_issue) 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. - _record_created(db, user, session, new) - db.commit() raise issue_refused(exc) from None except HTTPException: - raise # the pace, refused before anything was opened - except Exception: - # Anything else after the issue was opened must not lose the record that it was: a retry - # would open a second one. + raise # the pace, refused before the tracker was asked + finally: + _end_send(db, user, session, new) + 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.""" + try: + debugsession.lock(db, session) + debugsession.release_send(session) _record_created(db, user, session, new) db.commit() - raise - _record_created(db, user, session, new) - db.commit() - return {"ok": True, "session": _view(db, session)} + except Exception: # noqa: BLE001 — whatever went wrong here, the send's own failure is the news + db.rollback() + log.warning( + "a send could not give its claim back", + extra=logs.event("issues.claim.stuck", session_id=session.id), + ) def _record_created(db: Session, user: User, session: DebugSession, new: bool) -> None: @@ -315,6 +331,8 @@ def _screenshot_refused(code: str) -> HTTPException: return refused(409, "screenshot_already_sent") if code == "too_many": return refused(422, "screenshot_too_many", limit=debugsession.SCREENSHOTS_MAX) + if code == "sending": + return refused(409, "issue_sending") if code == "storage_full": mb = debugsession.SCREENSHOT_USER_MAX_BYTES // (1024 * 1024) return refused(422, "screenshot_storage_full", limit=mb) diff --git a/backend/tests/test_debug_sessions.py b/backend/tests/test_debug_sessions.py index f1ce0f0f..0a9c0837 100644 --- a/backend/tests/test_debug_sessions.py +++ b/backend/tests/test_debug_sessions.py @@ -300,11 +300,22 @@ def test_a_request_without_the_header_keeps_the_sessions_floor(gated): assert root.level == logging.INFO +def test_an_erased_account_takes_its_session_elevation_with_it(gated): + until = datetime.now(timezone.utc) + timedelta(minutes=30) + request = contextvars.copy_context() + request.run(logs.set_user, 11, "TRACE", until, "b" * 16) + assert logging.getLogger().level == logs.resolve_level("TRACE") + logs.forget_user(11) + assert logs._elevations == {} and logging.getLogger().level == logging.INFO + + def test_a_session_never_lowers_the_users_own_level(): now = datetime.now(timezone.utc) user = User(diagnostics_level="TRACE", diagnostics_until=now + timedelta(hours=3)) session = DebugSession(level="DEBUG", until=now + timedelta(minutes=30)) - assert debugsession.level_for(user, session) == ("TRACE", user.diagnostics_until) + # The user's own level, but never past the recording: the process tracks this pair under the + # session, and an expiry outliving it would hold the root logger down after it ended. + assert debugsession.level_for(user, session) == ("TRACE", session.until) assert debugsession.level_for(User(), session) == ("DEBUG", session.until) assert debugsession.level_for(User(), None) == ("INFO", None) @@ -480,14 +491,45 @@ def test_a_screenshot_can_be_removed_and_is_then_gone(recording): assert r.status_code == 404 and r.json()["detail"]["reason"] == "screenshot" -def test_a_sent_session_keeps_its_screenshots(db, recording): +def test_a_half_sent_session_keeps_its_screenshots_a_finished_one_lets_them_go(db, recording): client, sid = recording shot = _upload(client, sid, _PNG).json()["screenshot"]["id"] - db.get(DebugSession, sid).issue_url = "https://git.example/o/r/issues/1" + session = db.get(DebugSession, sid) + session.issue_url = "https://git.example/o/r/issues/1" + debugsession.set_delivery(session, {"number": 1, "pending": ["server.log"], "links": {}}) db.commit() - # The issue carries it already, and a half-sent one names its screenshots by their order. + # A send that still owes files names its screenshots by their order: removing one renumbers + # what it has left to send. r = client.delete(f"/api/debug-sessions/{sid}/screenshots/{shot}") assert r.status_code == 409 and r.json()["detail"]["reason"] == "screenshot_already_sent" + # Finished: the issue has its own copy, and ours is the user's to drop. + debugsession.set_delivery(session, {"number": 1, "pending": [], "links": {}}) + db.commit() + 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): + client, sid = recording + 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" + r = client.post(f"/api/debug-sessions/{sid}/issue") + assert r.status_code == 409 and r.json()["detail"]["reason"] == "issue_sending" + # 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()}, + ) + db.commit() + assert ( + client.put(f"/api/debug-sessions/{sid}/draft", json={"title": "x", "body": "y"}).status_code + == 200 + ) def test_a_users_screenshots_are_capped_across_their_sessions(recording, monkeypatch): @@ -814,10 +856,12 @@ def test_a_failed_edit_that_shows_the_screenshots_is_finished_by_the_retry( _ready(client, sid) r = client.post(f"/api/debug-sessions/{sid}/issue") assert r.status_code == 422 and r.json()["detail"]["reason"] == "issues_unreachable" - # Every file is up, but the body does not show the screenshot yet: the send is not finished. - assert client.get(f"/api/debug-sessions/{sid}").json()["pending_files"] == ["screenshot-1.png"] + # Every file is up, but the body does not show the screenshot yet: the send is not finished — + # and nothing is named as an attachment that did not go, because they all did. + session = client.get(f"/api/debug-sessions/{sid}").json() + assert (session["pending_files"], session["images_pending"]) == ([], True) r = client.post(f"/api/debug-sessions/{sid}/issue") - assert r.status_code == 200 and r.json()["session"]["pending_files"] == [] + assert r.status_code == 200 and r.json()["session"]["images_pending"] is False assert "![screenshot-1.png](https://git.example/a/screenshot-1.png)" in gitea_fake.patched_body assert len(gitea_fake.uploads()) == 3 # nothing uploaded twice @@ -837,18 +881,23 @@ 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_a_redirecting_tracker_is_refused_not_a_crash(gitea_fake, db, make_user, as_user): +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")) - r = admin.post("/api/admin/config/test-issues") + with caplog.at_level(logging.WARNING): + r = admin.post("/api/admin/config/test-issues") assert r.status_code == 422 and r.json()["detail"]["reason"] == "issues_rejected" + # Refused as an ANSWER — only the API's own 2xx is one. A 3xx read as success would be handed to + # the parser instead, and the tracker's address would look merely unparseable. + events = [e for x in caplog.records if (e := getattr(x, "event", "")).startswith("issues.")] + assert events == ["issues.gitea.refused"] def test_the_delivery_survives_a_save_of_the_draft(): session = DebugSession(draft={"title": "t", "delivery": {"number": 7, "pending": ["a.log"]}}) debugsession.save_draft(session, {"title": "t2", "body": "b", "excluded": [], "form": {}}) assert session.draft["title"] == "t2" - assert debugsession.unfinished(session) == ["a.log"] + assert debugsession.not_uploaded(session) == ["a.log"] def test_a_report_needs_a_title_and_a_wrong_token_says_so(gitea_fake, db, client, make_user): diff --git a/backend/tests/test_route_conventions.py b/backend/tests/test_route_conventions.py index a7ccefcd..c3850fe0 100644 --- a/backend/tests/test_route_conventions.py +++ b/backend/tests/test_route_conventions.py @@ -28,6 +28,8 @@ COMMITTING_HELPERS = { "commits nothing)", "messages.py:ensure_welcome": "the websocket push after it must announce a row the client " "can already fetch", + "debug_sessions.py:_end_send": "gives back the claim a send took, in a `finally`: it runs when " + "the handler is already unwinding and has no commit left to reach", } diff --git a/frontend/src/components/DebugRecorder.dom.test.tsx b/frontend/src/components/DebugRecorder.dom.test.tsx index b19e7025..f646ea72 100644 --- a/frontend/src/components/DebugRecorder.dom.test.tsx +++ b/frontend/src/components/DebugRecorder.dom.test.tsx @@ -38,6 +38,7 @@ const { order, session, startDebugSession, stopDebugSession, hold } = vi.hoisted ended_reason: null, issue_url: null, pending_files: [], + images_pending: false, screenshots: 0, draft: null, ...over, @@ -79,6 +80,7 @@ import { setStepPrompt, useComposerSession, } from "../lib/debugRecorder"; +import { anyBlockingLayer } from "../lib/layerStack"; import { events } from "../lib/log"; beforeEach(() => { @@ -215,6 +217,17 @@ 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", () => { + mount({ debug_session: session() }); + expect(anyBlockingLayer()).toBe(false); + 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. + 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", () => { mount({ debug_session: session() }, ); fireEvent.click(within(pill()!).getByRole("button", { name: "recorder.pill.step" })); diff --git a/frontend/src/components/DebugRecorder.tsx b/frontend/src/components/DebugRecorder.tsx index 784ea066..9d47ef18 100644 --- a/frontend/src/components/DebugRecorder.tsx +++ b/frontend/src/components/DebugRecorder.tsx @@ -23,6 +23,7 @@ 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"; @@ -286,14 +287,15 @@ 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. - // Passive: it takes Escape, not the Tab — the dialog under it keeps its focus trap, which spans - // its toast host and so this prompt too; a blocking layer would switch that trap off and let Tab - // walk out of the dialog into the page behind its scrim. - useLayer(true, () => onDone(null), { passive: true }); + // 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)); const onKeyDown = (e: ReactKeyboardEvent) => { if (e.key === "Enter") { e.preventDefault(); @@ -301,7 +303,13 @@ 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" + > diff --git a/frontend/src/components/IssueComposer.dom.test.tsx b/frontend/src/components/IssueComposer.dom.test.tsx index c0363bb6..165c3f26 100644 --- a/frontend/src/components/IssueComposer.dom.test.tsx +++ b/frontend/src/components/IssueComposer.dom.test.tsx @@ -65,7 +65,10 @@ vi.mock("../lib/api", async (importOriginal) => { screenshot: { id: 1, mime: image.type, size: 4, created_at: "" }, }); }, - debugExportUrl: () => "/api/debug-sessions/0123456789abcdef/export", + debugExport: () => { + api.order.push("export"); + return Promise.resolve({ blob: new Blob(["zip"]), name: "siftlode-debug.zip" }); + }, sendDebugIssue: async () => { api.order.push("send"); if (api.holdSend) await api.holdSend; @@ -115,6 +118,7 @@ const baseSession = (over: Partial = {}): DebugSession => ({ ended_reason: "stopped", issue_url: null, pending_files: [], + images_pending: false, screenshots: 0, draft: null, ...over, @@ -264,17 +268,42 @@ describe("the issue composer", () => { expect(screen.queryByText("recorder.composer.saved")).toBeNull(); }); - it("says so when the screenshots cannot be read, and saves nothing that would miscount them", async () => { + it("says so when the screenshots cannot be read, and still counts them from the session", async () => { api.failShots = true; + api.session = baseSession({ screenshots: 2 }); mount(); expect(await screen.findByText("recorder.composer.shotsFailed")).not.toBeNull(); - expect( - (screen.getByRole("button", { name: "recorder.composer.export" }) as HTMLButtonElement) - .disabled, - ).toBe(true); + // The draft is still saved — and says two screenshots, the server's own count, not none. fireEvent.change(title(), { target: { value: "x" } }); - await new Promise((r) => setTimeout(r, 1000)); - expect(api.saved).toHaveLength(0); + await waitFor(() => expect(api.saved).toHaveLength(1), { timeout: 2000 }); + expect(lastSaved().body).toContain("recorder.issue.screenshots:2"); + }); + + it("leaves an image pasted into a dialog opened over it alone", async () => { + mount(); + await screen.findByText("recorder.composer.attachments"); + const above = document.createElement("div"); + above.setAttribute("aria-modal", "true"); + const field = document.createElement("input"); + above.appendChild(field); + document.body.appendChild(above); + const png = new File([new Uint8Array([137, 80, 78, 71])], "shot.png", { type: "image/png" }); + fireEvent.paste(field, { clipboardData: { files: [png] } }); + await new Promise((r) => setTimeout(r, 50)); + expect(api.uploaded).toHaveLength(0); + above.remove(); + }); + + it("says when the issue is there but does not show its screenshots yet", async () => { + tracker = { repo: "o/r" }; + api.session = baseSession({ + issue_url: "https://git.example/o/r/issues/7", + images_pending: true, + }); + mount(); + expect(await screen.findByText("recorder.composer.partialImages")).not.toBeNull(); + fireEvent.click(screen.getByRole("button", { name: "recorder.composer.retry" })); + await waitFor(() => expect(api.sent).toBe(1)); }); it("moves the focus to what stays when a control goes", async () => { @@ -302,13 +331,21 @@ describe("the issue composer", () => { expect(api.uploaded).toHaveLength(0); }); - it("saves before it downloads the zip", async () => { - const click = vi.spyOn(HTMLAnchorElement.prototype, "click").mockImplementation(() => {}); + it("saves before it asks for the zip, and hands it to the browser", async () => { + const click = vi.spyOn(HTMLAnchorElement.prototype, "click"); click.mockImplementation(() => void api.order.push("click")); + const urls = { made: 0, revoked: 0 }; + URL.createObjectURL = () => { + urls.made += 1; + return "blob:zip"; + }; + URL.revokeObjectURL = () => void (urls.revoked += 1); mount(); fireEvent.click(await ready("recorder.composer.export")); await waitFor(() => expect(click).toHaveBeenCalled()); - expect(api.order).toEqual(["save", "saved", "click"]); + // A request, not a bare navigation: a refusal (the export's own pace) is shown like any other. + expect(api.order).toEqual(["save", "saved", "export", "click"]); + expect([urls.made, urls.revoked]).toEqual([1, 1]); click.mockRestore(); }); diff --git a/frontend/src/components/IssueComposer.tsx b/frontend/src/components/IssueComposer.tsx index 27e2550d..5a083129 100644 --- a/frontend/src/components/IssueComposer.tsx +++ b/frontend/src/components/IssueComposer.tsx @@ -128,10 +128,10 @@ function ComposerBody({ queryKey: qk.debugScreenshots(session.id), queryFn: () => api.debugScreenshots(session.id), }); - // Until the screenshots are known, the body cannot say how many there are: nothing is saved or - // sent from it before then (a failed read must not become "no screenshots" in the issue). - const shotsReady = shotsQ.data !== undefined; const shots = shotsQ.data?.items ?? []; + // How many the ISSUE will carry. From the list while it is loaded, else from the session itself + // (the server's own count) — a failed read must not quietly become "no screenshots". + const shotCount = shotsQ.data ? shots.length : session.screenshots; const excluded = useMemo(() => new Set(form.excluded), [form.excluded]); const kept = (source: Source) => @@ -155,7 +155,7 @@ function ComposerBody({ const body = renderIssue( form, env, - { client: kept("web"), server: kept("server"), screenshots: shots.length }, + { client: kept("web"), server: kept("server"), screenshots: shotCount }, text, ); @@ -213,14 +213,14 @@ function ComposerBody({ const saveRef = useRef(save.mutate); saveRef.current = save.mutate; useEffect(() => { - if (locked || !unsaved || !shotsReady) return; + if (locked || !unsaved) return; const timer = window.setTimeout(() => saveRef.current(), SAVE_DELAY_MS); return () => window.clearTimeout(timer); - }, [body, form.title, locked, unsaved, shotsReady]); + }, [body, form.title, locked, unsaved]); // Closed before the typing paused: save what is still waiting, now. const flushRef = useRef(() => {}); flushRef.current = () => { - if (unsaved && !locked && shotsReady) save.mutate(); + if (unsaved && !locked) save.mutate(); }; useEffect(() => () => flushRef.current(), []); @@ -233,10 +233,14 @@ function ComposerBody({ }); const addRef = useRef(addShot.mutate); addRef.current = addShot.mutate; + const root = useRef(null); useEffect(() => { if (locked) return; // Only an image paste is taken over: text pasted into the title or a step goes where it was typed. + // And only OUR dialog's: something opened over the composer (the command palette) keeps its own. const onPaste = (e: ClipboardEvent) => { + const dialog = e.target instanceof Element ? e.target.closest('[aria-modal="true"]') : null; + if (dialog && !dialog.contains(root.current)) return; const images = [...(e.clipboardData?.files ?? [])].filter(isImage); if (!images.length) return; e.preventDefault(); @@ -248,19 +252,21 @@ function ComposerBody({ const [mode, setMode] = useState<"edit" | "preview">("edit"); // The export is built from the SAVED draft: save what is on screen first. - const exportZip = async () => { - if (!sent) { - try { - await save.mutateAsync(); - } catch { - return; // the save's own message says why - } - } - const a = document.createElement("a"); - a.href = api.debugExportUrl(session.id); - a.download = ""; - a.click(); - }; + const exportZip = useMutation({ + mutationFn: async () => { + if (!sent) await save.mutateAsync(); + return api.debugExport(session.id); + }, + meta: { failTitle: "recorder.composer.exportFailed" }, + onSuccess: ({ blob, name }) => { + const url = URL.createObjectURL(blob); + const a = document.createElement("a"); + a.href = url; + a.download = name; + a.click(); + URL.revokeObjectURL(url); + }, + }); const started = new Date(session.started_at); const minutes = Math.max( @@ -271,7 +277,7 @@ function ComposerBody({ ); return ( -
+

{t("recorder.composer.meta", { start: started.toLocaleString(i18n.language), @@ -279,9 +285,13 @@ function ComposerBody({ level: t(`settings.diagnostics.levels.${session.level}`), })}

- {sent && pending.length > 0 && ( + {sent && (pending.length > 0 || session.images_pending) && (
- {t("recorder.composer.partial", { files: pending.join(", ") })} + + {pending.length > 0 + ? t("recorder.composer.partial", { files: pending.join(", ") }) + : t("recorder.composer.partialImages")} + {target && !sent && ( - )} @@ -647,7 +652,7 @@ function Attachments({ )}
))} - {!readOnly && shotsQ.data && ( + {!readOnly && (
e.preventDefault()} onDrop={onDrop} diff --git a/frontend/src/components/ui/Toaster.dom.test.tsx b/frontend/src/components/ui/Toaster.dom.test.tsx index d97a93a7..e5fab134 100644 --- a/frontend/src/components/ui/Toaster.dom.test.tsx +++ b/frontend/src/components/ui/Toaster.dom.test.tsx @@ -15,6 +15,7 @@ vi.mock("react-i18next", async (importOriginal) => { import Toaster from "./Toaster"; import { clearAll, notify } from "../../lib/notifications"; +import { dockUnderToasts } from "../../lib/toastDock"; afterEach(() => { cleanup(); @@ -50,12 +51,21 @@ describe("Toaster — one host shows the toasts", () => { app.unmount(); }); - it("lets a click through wherever there is no toast and nothing docked, in every host", () => { + it("lets a click through where there is no toast and nothing docked, and not where there is", () => { + const Docked = () => ; + const stop = dockUnderToasts(Docked); + notify({ level: "info", message: "hello" }); render(); const region = document.querySelector("[data-toast-region]")!; // The empty part of the region (beside and above a docked control) must not swallow the page's - // clicks; the cards and the dock opt back in. + // clicks — and the two things that ARE there must still take them. expect(region.className).toContain("pointer-events-none"); + const card = screen.getByText("hello").closest(".toast-card")!; + expect(card.className).toContain("pointer-events-auto"); + const dock = screen.getByText("dock").parentElement!; + expect(dock.className).toContain("pointer-events-auto"); + expect(dock.closest("[data-toast-region]")).toBe(region); + stop(); }); it("does not let a click on a toast reach the overlay's backdrop", () => { diff --git a/frontend/src/i18n/locales/en/errors.json b/frontend/src/i18n/locales/en/errors.json index b5cd0c16..d57f8177 100644 --- a/frontend/src/i18n/locales/en/errors.json +++ b/frontend/src/i18n/locales/en/errors.json @@ -225,6 +225,7 @@ "issues_rate_limited": "You can send at most {{limit}} reports an hour. Try again later.", "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." + "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." } } diff --git a/frontend/src/i18n/locales/en/recorder.json b/frontend/src/i18n/locales/en/recorder.json index 04825399..00681146 100644 --- a/frontend/src/i18n/locales/en/recorder.json +++ b/frontend/src/i18n/locales/en/recorder.json @@ -107,7 +107,9 @@ "previewTitle": "Preview of the report", "previewLocal": "Without an issue tracker, this is the zip's issue.md.", "shotsFailed": "Couldn't load the screenshots", - "removeShotFailed": "Couldn't remove the screenshot" + "removeShotFailed": "Couldn't remove the screenshot", + "partialImages": "The issue was created, but its text does not show the screenshots yet.", + "exportFailed": "Couldn't build the zip" }, "issue": { "steps": "Steps", diff --git a/frontend/src/i18n/locales/hu/errors.json b/frontend/src/i18n/locales/hu/errors.json index 290337a6..fa6de540 100644 --- a/frontend/src/i18n/locales/hu/errors.json +++ b/frontend/src/i18n/locales/hu/errors.json @@ -225,6 +225,7 @@ "issues_rate_limited": "Óránként legfeljebb {{limit}} hibajegyet küldhetsz. Próbáld újra később.", "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." + "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." } } diff --git a/frontend/src/i18n/locales/hu/recorder.json b/frontend/src/i18n/locales/hu/recorder.json index 871d1ae6..fba6689d 100644 --- a/frontend/src/i18n/locales/hu/recorder.json +++ b/frontend/src/i18n/locales/hu/recorder.json @@ -107,7 +107,9 @@ "previewTitle": "A hibajegy előnézete", "previewLocal": "Hibajegy-kezelő nélkül ez a zip issue.md fájlja.", "shotsFailed": "Nem sikerült betölteni a képernyőképeket", - "removeShotFailed": "Nem sikerült törölni a képernyőképet" + "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.", + "exportFailed": "Nem sikerült elkészíteni a zipet" }, "issue": { "steps": "Lépések", diff --git a/frontend/src/lib/api/core.ts b/frontend/src/lib/api/core.ts index 3f18f00c..f090420e 100644 --- a/frontend/src/lib/api/core.ts +++ b/frontend/src/lib/api/core.ts @@ -225,6 +225,8 @@ function reportFailure(err: HttpError, quiet: boolean): void { // are safe to repeat). Used to recover from the keepalive race without surfacing a 502. export interface ReqConfig { idempotent?: boolean; + // Return the `Response` itself instead of parsed JSON — for a file (see the return below). + raw?: boolean; // Suppress the global error modal for 4xx/5xx so the caller can show the error inline // (used by the auth forms — a wrong password shouldn't pop a blocking dialog). quiet?: boolean; @@ -358,6 +360,10 @@ 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. + 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. return (r.status === 204 ? null : await r.json()) as T; diff --git a/frontend/src/lib/api/diagnostics.ts b/frontend/src/lib/api/diagnostics.ts index 2f23d710..92197581 100644 --- a/frontend/src/lib/api/diagnostics.ts +++ b/frontend/src/lib/api/diagnostics.ts @@ -55,6 +55,8 @@ export interface DebugSession { issue_url: string | null; /** Files a send that stopped half-way has still to deliver (retrying sends only these). */ pending_files: string[]; + /** Whether the issue's body has still to be edited to show the screenshots it already carries. */ + images_pending: boolean; screenshots: number; /** The issue being written from it (the composer saves as it goes); null until it first does. */ draft: DebugDraft | null; @@ -102,12 +104,13 @@ export const debugSessionsApi = { }), saveDebugDraft: (id: string, draft: DebugDraft): Promise<{ ok: true; session: DebugSession }> => req(`${SESSIONS}/${id}/draft`, { method: "PUT", body: JSON.stringify(draft) }), - /** The issue as a zip, built from the SAVED draft. A navigation, not a `req` — so the account - * rides as `?account=`, as for every download. */ - debugExportUrl: (id: string): string => { - const a = getActiveAccount(); - const url = `${SESSIONS}/${id}/export`; - return a != null ? `${url}?account=${a}` : url; + /** The report as a zip, built from the SAVED draft — a request, not a navigation, so a refusal + * (the export's own pace) reaches the user as a message instead of a saved error page. */ + debugExport: async (id: string): Promise<{ blob: Blob; name: string }> => { + const r = await req(`${SESSIONS}/${id}/export`, {}, { raw: true }); + const disposition = r.headers.get("content-disposition") ?? ""; + const name = /filename="([^"]+)"/.exec(disposition)?.[1] ?? `siftlode-debug-${id}.zip`; + return { blob: await r.blob(), name }; }, /** Send the SAVED draft to the issue tracker, or finish a send that stopped half-way. */ sendDebugIssue: (id: string): Promise<{ ok: true; session: DebugSession }> => diff --git a/frontend/src/lib/diagnostics.session.dom.test.ts b/frontend/src/lib/diagnostics.session.dom.test.ts index f96e06aa..3036a4d1 100644 --- a/frontend/src/lib/diagnostics.session.dom.test.ts +++ b/frontend/src/lib/diagnostics.session.dom.test.ts @@ -38,6 +38,7 @@ const recording = (level: "DEBUG" | "TRACE", minutes = 60, over = {}) => ({ ended_reason: null, issue_url: null, pending_files: [], + images_pending: false, screenshots: 0, draft: null, ...over,