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,