peter/siftlode
1
0
Fork 0
Code Issues Pull Requests Packages Projects Releases Wiki Activity

fix(diagnostics): deep-review round 3 — a keyboard owner that is not a Tab trap

Browse Source 
Round 2 made the step prompt a blocking layer so that `j` and the `g` chords
would not reach the page behind it. The dialog it sits in gave up its Tab trap
because of that, while the prompt's own replacement trap covered only its own
two controls — so Tab from anywhere else in the dialog (the bar's Screenshot or
Stop, the dialog's own buttons) walked out behind the scrim. `layerStack` now
separates the two questions it had folded into one flag: a layer can own the
keyboard and still leave the Tab trap to the dialog below it. The mirror case,
a dialog opening over the prompt, falls out of the same split.

A send's claim is now a lease with an owner. Every progress report re-stamps
it, so a send slower than the lease cannot lose it while it is still running,
and only the send that took a claim can give it back. The issue_create audit
row is staged inside the send and rides the same checkpoint as `issue_url`, so
the trail cannot be missing for an issue that exists. The attachment-name
prefix has one definition, shared by the producer and both readers.

A running send is reported to the UI. A reload or a second tab used to render
it exactly like a send that STOPPED, and offered a retry the server was certain
to refuse; the composer now says the report is being sent, holds the draft, and
puts both the retry and Create issue out. The past-sessions list says the same
for the same reason — it is the list a user opens to check on a send, and both
"draft" and "half sent" describe one that died. `/preview` and the retry path
got a pace of their own, and the per-user byte cap took the user-scoped lock
that makes it a cap instead of a guess.

Verified in a real browser on localdev: Tab cycles the whole dialog with the
prompt open, the prompt still swallows the page's keys, and the issue link is
focusable where the composer puts it. All 24 regression tests mutation-checked.
This commit is contained in:
peter
2026-09-20 02:43:56 +02:00
parent bc03e82687
commit 2fe58073e4
20 changed files with 718 additions and 137 deletions
Download Patch File Download Diff File Expand all files Collapse all files
frontend/src/components/DebugRecorder.dom.test.tsx
+96 -10
View File
@@ -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<void> | 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<typeof import("../lib/api")>();
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<typeof import("../lib/diagnostics")>();
@@ -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(
<QueryClientProvider client={qc}>
<RecorderSettings />
</QueryClientProvider>,
);
}
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();
});
});
frontend/src/components/DebugRecorder.tsx
+31 -22
View File
@@ -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<HTMLDivElement>(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<HTMLInputElement>) => {
if (e.key === "Enter") {
e.preventDefault();
@@ -303,13 +303,7 @@ function StepPrompt({ id, onDone }: { id: string; onDone: (label: string | null)
}
};
return (
<div
ref={box}
onKeyDown={(e) => {
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"
>
<div 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">
<label htmlFor="debug-step-label" className="text-sm text-muted">
{t("recorder.pill.stepPrompt", { n })}
</label>
@@ -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() {
<span className="min-w-0 flex-1 truncate text-muted">
{s.draft?.title || t(`settings.diagnostics.levels.${s.level}`)}
</span>
<span className={`chip px-2 py-0.5 text-2xs ${s.issue_url ? "text-success" : ""}`}>
{t(
s.issue_url
? "recorder.history.sent"
: s.draft
? "recorder.history.draft"
: "recorder.history.unwritten",
)}
<span className={`chip px-2 py-0.5 text-2xs ${chipTone(s)}`}>
{t(historyState(s))}
</span>
<Button variant="outline" size="xs" onClick={() => openComposer(s.id)}>
{t("recorder.history.open")}
frontend/src/components/IssueComposer.dom.test.tsx
+72 -13
View File
@@ -119,6 +119,7 @@ const baseSession = (over: Partial<DebugSession> = {}): DebugSession => ({
issue_url: null,
pending_files: [],
images_pending: false,
sending: false,
screenshots: 0,
draft: null,
...over,
@@ -160,7 +161,7 @@ function mount() {
const lastSaved = () => api.saved[api.saved.length - 1]!;
const title = () => screen.getByLabelText("recorder.composer.fieldTitle") as HTMLInputElement;
/** A button once it can be pressed: the export and the send wait for the screenshots. */
/** A button once it can be pressed: `loading` holds it while the draft's first save settles. */
async function ready(name: string): Promise<HTMLButtonElement> {
const button = (await screen.findByRole("button", { name })) as HTMLButtonElement;
await waitFor(() => expect(button.disabled).toBe(false));
@@ -273,12 +274,77 @@ describe("the issue composer", () => {
api.session = baseSession({ screenshots: 2 });
mount();
expect(await screen.findByText("recorder.composer.shotsFailed")).not.toBeNull();
// Everything still works: the images are the one thing that could not be listed.
expect(screen.queryByText("recorder.composer.attachments")).not.toBeNull();
expect(
(screen.getByRole("button", { name: "recorder.composer.export" }) as HTMLButtonElement)
.disabled,
).toBe(false);
// Including the control standing right beside the failed list: more can still be added.
expect(screen.queryByText("recorder.composer.pick")).not.toBeNull();
// The draft is still saved — and says two screenshots, the server's own count, not none.
fireEvent.change(title(), { target: { value: "x" } });
await waitFor(() => expect(api.saved).toHaveLength(1), { timeout: 2000 });
expect(lastSaved().body).toContain("recorder.issue.screenshots:2");
});
it("says a send is under way instead of calling it half-failed, and holds the draft", async () => {
// Every step of a send commits its progress, so a reload or a second tab reads exactly the
// shape of a send that STOPPED. Only the server can tell the two apart — and it does.
api.session = baseSession({
issue_url: "https://git.example/o/r/issues/7",
pending_files: ["server.log", "screenshot-1.png"],
sending: true,
});
mount();
expect(await screen.findByText("recorder.composer.sendingNow")).not.toBeNull();
expect(screen.queryByText(/recorder.composer.partial/)).toBeNull();
// No retry to press: it would only be refused while the send it invites is still running.
expect(screen.queryByRole("button", { name: "recorder.composer.retry" })).toBeNull();
fireEvent.change(title(), { target: { value: "typed while it went out" } });
await new Promise((r) => setTimeout(r, 1000));
expect(api.saved).toHaveLength(0);
});
it("holds the draft through a first send, before there is an issue to point at", async () => {
tracker = { repo: "o/r" };
// A send with no issue_url yet is what the read-only lock is for: an edit made mid-flight
// would be saved over the very body the send is at that moment uploading, and the issue that
// arrives would be neither draft. The half-sent case above is locked by its link instead.
api.session = baseSession({ sending: true });
mount();
expect(await screen.findByText("recorder.composer.sendingNow")).not.toBeNull();
fireEvent.change(title(), { target: { value: "typed mid-flight" } });
await new Promise((r) => setTimeout(r, 1000));
expect(api.saved).toHaveLength(0);
// Nor can a screenshot join what is already on its way up.
expect(screen.queryByText("recorder.composer.pick")).toBeNull();
// And the button that would start a SECOND send is out — the server would only refuse it.
expect(
(screen.getByRole("button", { name: "recorder.composer.send" }) as HTMLButtonElement)
.disabled,
).toBe(true);
});
it("asks to show the images, not to send what is already there", async () => {
tracker = { repo: "o/r" };
api.session = baseSession({
issue_url: "https://git.example/o/r/issues/7",
images_pending: true,
});
mount();
await screen.findByText("recorder.composer.partialImages");
// Nothing is missing from the issue here — only its text has still to show what it carries.
expect(screen.queryByRole("button", { name: "recorder.composer.retry" })).toBeNull();
fireEvent.click(screen.getByRole("button", { name: "recorder.composer.retryImages" }));
await waitFor(() => expect(api.sent).toBe(1));
// The button just pressed is gone with its banner: the focus goes to what takes its place,
// not to <body> in the middle of an open dialog.
await waitFor(() =>
expect(document.activeElement?.getAttribute("href")).toBe("https://git.example/o/r/issues/7"),
);
});
it("leaves an image pasted into a dialog opened over it alone", async () => {
mount();
await screen.findByText("recorder.composer.attachments");
@@ -294,18 +360,6 @@ describe("the issue composer", () => {
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 () => {
mount();
fireEvent.click(
@@ -335,6 +389,9 @@ describe("the issue composer", () => {
const click = vi.spyOn(HTMLAnchorElement.prototype, "click");
click.mockImplementation(() => void api.order.push("click"));
const urls = { made: 0, revoked: 0 };
// jsdom implements neither, so there is nothing to spy on: they are DEFINED here — and put
// back afterwards, or every later test in this file would keep running against them.
const real = { make: URL.createObjectURL, revoke: URL.revokeObjectURL };
URL.createObjectURL = () => {
urls.made += 1;
return "blob:zip";
@@ -347,6 +404,8 @@ describe("the issue composer", () => {
expect(api.order).toEqual(["save", "saved", "export", "click"]);
expect([urls.made, urls.revoked]).toEqual([1, 1]);
click.mockRestore();
URL.createObjectURL = real.make;
URL.revokeObjectURL = real.revoke;
});
it("a sent session is read-only and links its issue", async () => {
frontend/src/components/IssueComposer.tsx
+41 -15
View File
@@ -120,6 +120,7 @@ function ComposerBody({
},
);
const [reviewing, setReviewing] = useState<Source | null>(null);
const issueLink = useRef<HTMLAnchorElement>(null);
// The log whose review just closed: its Review button takes the focus back.
const reviewed = useRef<Source | null>(null);
const sent = session.issue_url !== null;
@@ -192,6 +193,9 @@ function ComposerBody({
// While it runs the draft is held as it was saved: nothing typed then would reach the issue.
const target = useMe()?.issues ?? null;
const pending = session.pending_files;
// A send is running — this tab's or another's. Its progress is committed step by step, so the
// half-sent shape below is also what a LIVE send looks like; only the server can tell them apart.
const sending = session.sending;
const send = useMutation({
mutationFn: async () => {
if (!sent) await save.mutateAsync();
@@ -201,6 +205,9 @@ function ComposerBody({
onSuccess: (r) => {
qc.setQueryData(qk.debugSession(session.id), r.session);
notify({ level: "success", message: t("recorder.composer.sentOk") });
// The Retry that was just pressed lives in a banner that this answer unmounts. Hand the focus
// to the link that takes its place, the way every other control-that-goes here does.
window.setTimeout(() => issueLink.current?.focus(), 0);
},
// A failure half-way still created the issue: show what the server now has.
onSettled: () => {
@@ -208,7 +215,7 @@ function ComposerBody({
void qc.invalidateQueries({ queryKey: qk.debugSession(session.id), exact: true });
},
});
const locked = sent || send.isPending;
const locked = sent || send.isPending || sending;
const saveRef = useRef(save.mutate);
saveRef.current = save.mutate;
@@ -285,27 +292,38 @@ function ComposerBody({
level: t(`settings.diagnostics.levels.${session.level}`),
})}
</p>
{sent && (pending.length > 0 || session.images_pending) && (
<div className="flex flex-wrap items-center justify-between gap-2 rounded-xl border border-warn/40 bg-warn/10 px-3 py-2 text-sm">
{(sending || (sent && (pending.length > 0 || session.images_pending))) && (
<div
className={`flex flex-wrap items-center justify-between gap-2 rounded-xl border px-3 py-2 text-sm ${
sending ? "border-accent/40 bg-accent/10" : "border-warn/40 bg-warn/10"
}`}
aria-live="polite"
>
<span>
{pending.length > 0
? t("recorder.composer.partial", { files: pending.join(", ") })
: t("recorder.composer.partialImages")}
{sending
? t("recorder.composer.sendingNow")
: pending.length > 0
? t("recorder.composer.partial", { files: pending.join(", ") })
: t("recorder.composer.partialImages")}
</span>
<Button
variant="outline"
size="sm"
loading={send.isPending}
onClick={() => send.mutate()}
>
{t("recorder.composer.retry")}
</Button>
{/* Nothing to retry while it is still going — the button would only be refused. */}
{!sending && (
<Button
variant="outline"
size="sm"
loading={send.isPending}
onClick={() => send.mutate()}
>
{t(pending.length > 0 ? "recorder.composer.retry" : "recorder.composer.retryImages")}
</Button>
)}
</div>
)}
{sent && (
<div className="flex flex-wrap items-center justify-between gap-2 rounded-xl border border-success/40 bg-success/10 px-3 py-2 text-sm">
<span>{t("recorder.composer.sent")}</span>
<a
ref={issueLink}
href={session.issue_url!}
target="_blank"
rel="noreferrer"
@@ -401,7 +419,15 @@ function ComposerBody({
{t("recorder.composer.export")}
</Button>
{target && !sent && (
<Button variant="primary" loading={send.isPending} onClick={() => send.mutate()}>
// `sending` is a send ALREADY under way — from a reload, or from the other tab this one
// is a copy of. The button would only earn an `issue_sending` refusal; the banner above
// says why it is out.
<Button
variant="primary"
loading={send.isPending}
disabled={sending}
onClick={() => send.mutate()}
>
{t("recorder.composer.send")}
</Button>
)}