commit e3c8e2b9aa13e69f1ae1841c76ff554531ccc0d2
parent 270f42ec052d0805ff7811ed0eb66bd740310698
Author: I Mean I'm Just Saying <imeanimjustsaying@kiwifarms.st>
Date: Fri, 18 Sep 2026 17:43:06 -0400
clip bench: e2e for the attribution, the walk and the segment route
Against bench-fixture, whose cue file says `Fixture source vid1` / `20250101` —
so the line the renderer would draw for an un-overridden clip is exact rather
than plausible, and so is what changes when a clip overrides it.
Covered: the preview is build-video's own attributionLine and the fields rewrite
it; an edit survives a reload because it is the manifest being read; an empty
value deletes the key; 2025-02-31 is refused with what you typed left in the box
to fix; prev/next by link and by key; the segment route 404s for a clip nobody
has rendered, serves 206 for one that exists, and caches only when `v` matches
the mtime; a correction persists and turns up on the project page, and clearing
it takes the section with it.
Two races fixed while writing them. Saves are queued behind one another and the
token is a ref: tab out of `title` into `date` and both blur handlers fire, and
two concurrent PUTs carrying the same token would 409 — the guard doing its job
against the only writer that cannot have lost anybody's judgement. And the
inputs stay live while a save is in flight, because disabling them steals the
focus the operator just moved.
Co-Authored-By: Claude Fable 5.1 <noreply@anthropic.com>
Diffstat:
3 files changed, 221 insertions(+), 10 deletions(-)
diff --git a/umtool/components/projects/ClipBench.tsx b/umtool/components/projects/ClipBench.tsx
@@ -139,7 +139,6 @@ const round2 = (n: number) => Number(n.toFixed(2));
export default function ClipBench({ data }: { data: ClipBenchData }) {
const [clip, setClip] = useState<Clip>(data.clip);
- const [token, setToken] = useState(data.token);
const [windows, setWindows] = useState<Win[]>(data.windows);
const [cues, setCues] = useState<Cue[]>(data.cues);
const [proposed, setProposed] = useState(data.proposed);
@@ -156,6 +155,12 @@ export default function ClipBench({ data }: { data: ClipBenchData }) {
const router = useRouter();
const video = useRef<HTMLVideoElement | null>(null);
+ // A REF, not state. Two saves can be in flight -- tab out of `title` straight
+ // into `date` and both blur handlers fire -- and the second must carry the
+ // token the first was given back, which a re-render has not delivered yet.
+ const token = useRef(data.token);
+ // And they must not interleave: same read-modify-write, one clip.
+ const saving = useRef<Promise<boolean>>(Promise.resolve(true));
const stopAt = useRef<number | null>(null);
// The widest cached file is the one the bench draws from: it is how much room
@@ -274,14 +279,14 @@ export default function ClipBench({ data }: { data: ClipBenchData }) {
}, [sel, clip.start, clip.end, onSel, play, router, data.project, data.prev, data.next]);
// ---- saving -------------------------------------------------------------
- const save = useCallback(
+ const doSave = useCallback(
async (patch: Record<string, unknown>) => {
setBusy("saving…");
setNote(null);
const r = await fetch("/api/report/window", {
method: "PUT",
headers: { "content-type": "application/json" },
- body: JSON.stringify({ project: data.project, clip: clip.id, token, ...patch }),
+ body: JSON.stringify({ project: data.project, clip: clip.id, token: token.current, ...patch }),
});
const j = (await r.json()) as Record<string, unknown>;
setBusy(null);
@@ -312,13 +317,31 @@ export default function ClipBench({ data }: { data: ClipBenchData }) {
for (const [k] of ATTRIB) if (patch[k] !== undefined) out[k] = attribValue(next, k);
return out;
});
- setToken(String(j.token ?? ""));
+ token.current = String(j.token ?? "");
setNote("saved");
// The widener's opinion changes when the window does.
void refresh();
return true;
},
- [data.project, clip, token],
+ [data.project, clip],
+ );
+
+ /**
+ * Queued behind whatever is already saving.
+ *
+ * Tab out of `title` straight into `date` and both blur handlers fire. Two
+ * concurrent PUTs would send the SAME token, and the second would come back a
+ * 409 -- the guard doing exactly its job, against the only writer that cannot
+ * possibly have lost anybody's judgement. So they go one at a time and the
+ * second reads the token the first was handed back.
+ */
+ const save = useCallback(
+ (patch: Record<string, unknown>) => {
+ const run = saving.current.catch(() => false).then(() => doSave(patch));
+ saving.current = run;
+ return run;
+ },
+ [doSave],
);
/** Persist one attribution field, on blur or Enter, if it actually changed. */
@@ -340,7 +363,7 @@ export default function ClipBench({ data }: { data: ClipBenchData }) {
setWindows(j.windows);
setCues(j.cues);
setProposed(j.proposed);
- setToken(j.token);
+ token.current = j.token;
setSegment(j.segment);
setSegmentMtime(j.segmentMtime);
if (j.windows[0]) setView({ from: j.windows[0].from, to: j.windows[0].to });
@@ -648,7 +671,6 @@ export default function ClipBench({ data }: { data: ClipBenchData }) {
name={k}
rows={2}
value={draft[k]}
- disabled={!!busy}
onChange={(e) => setDraft((d) => ({ ...d, [k]: e.target.value }))}
onBlur={() => commit(k)}
className="mt-1 w-full rounded border border-[var(--color-line)] bg-[var(--color-panel-2)] px-2 py-1 text-[12px]"
@@ -659,7 +681,6 @@ export default function ClipBench({ data }: { data: ClipBenchData }) {
name={k}
type="text"
value={draft[k]}
- disabled={!!busy}
placeholder={
k === "date"
? data.uploadDate
diff --git a/umtool/docs/clip-bench.md b/umtool/docs/clip-bench.md
@@ -154,6 +154,14 @@ show` pilled four ferret-rescue clips "ends mid-sentence" while the decisions
inbox stayed silent about them, because the inbox had a punctuation gate and the
detail did not. The inbox was right.
+**Two blur handlers can fire before either PUT returns.** Tab out of `title`
+straight into `date` and both save; both would carry the same token and the
+second would come back a 409 — the stale-token guard doing exactly its job,
+against the only writer that cannot have lost anybody's judgement. Saves are
+queued and the token is a ref, not state. Disabling the inputs while one is in
+flight was the other candidate fix, and it is worse: it steals the focus the
+operator just moved.
+
**A deleted key has to disappear from the form.** Merging a saved entry with
`{...clip, ...entry}` leaves the old `title` on screen after an empty save
deletes it — the write succeeded and the page said otherwise. The entry is
diff --git a/umtool/e2e/clip-bench.spec.ts b/umtool/e2e/clip-bench.spec.ts
@@ -1,6 +1,6 @@
import { test, expect } from "@playwright/test";
import { execFileSync } from "node:child_process";
-import { readFileSync, existsSync } from "node:fs";
+import { copyFileSync, existsSync, mkdirSync, readFileSync } from "node:fs";
import path from "node:path";
import { fileURLToPath } from "node:url";
@@ -36,11 +36,30 @@ const bench = (clip: string) => `/browse/${PROJECT}/clip/${clip}`;
const readClip = (id: string) => {
const m = JSON.parse(readFileSync(MANIFEST, "utf8")) as {
- timeline: { id: string; start: number; end: number; lockEnd?: boolean }[];
+ timeline: {
+ id: string;
+ start: number;
+ end: number;
+ lockEnd?: boolean;
+ title?: string;
+ date?: string;
+ correction?: string;
+ }[];
};
return m.timeline.find((e) => e.id === id)!;
};
+/** Type into an attribution field and let it save the way a blur does. */
+const setField = async (
+ page: import("@playwright/test").Page,
+ field: string,
+ value: string,
+) => {
+ const input = page.locator(`[data-attrib-field=${field}]`);
+ await input.fill(value);
+ await input.blur();
+};
+
const token = async (request: { get: (u: string) => Promise<{ json: () => Promise<unknown> }> }, clip: string) => {
const r = await request.get(`/api/report/clip?project=${encodeURIComponent(PROJECT)}&clip=${clip}`);
return (await r.json()) as { token: string };
@@ -207,3 +226,166 @@ test("a bench save shows up on the project page", async ({ page, request }) => {
// lockEnd is the acknowledgement, so the row's warning goes with it.
await expect(row).toHaveAttribute("data-mid-sentence", "0");
});
+
+
+// ---------------------------------------------------------------------------
+// The attribution.
+//
+// The fixture's cue file says `Fixture source vid1` / `20250101`, so the line
+// the renderer would draw for an un-overridden clip is exact rather than
+// plausible -- and so is what changes when a clip overrides it.
+// ---------------------------------------------------------------------------
+
+test("the preview is the line the renderer will draw, and the fields rewrite it", async ({
+ page,
+}) => {
+ await page.goto(bench("c02"));
+ const preview = page.locator("[data-attrib-preview]");
+ // Un-overridden: the archived record's own title (cleaned) and upload date,
+ // at `cite ?? start`. That is build-video.mjs's own attributionLine(),
+ // imported rather than re-implemented -- which is the point of asserting it
+ // here rather than in a unit test of the component.
+ await expect(preview).toHaveText("Fixture source vid1 · 2025-01-01 @ 0:09");
+
+ await setField(page, "title", "Monday Mail #11");
+ await setField(page, "date", "2016-09-12");
+ await expect(preview).toHaveText("Monday Mail #11 · 2016-09-12 @ 0:09");
+
+ // On disk, through the one writer. Polled rather than read once: a blur
+ // STARTS a PUT, and the preview above is local state until it lands.
+ await expect.poll(() => readClip("c02").title).toBe("Monday Mail #11");
+ await expect.poll(() => readClip("c02").date).toBe("2016-09-12");
+
+ // And it is the manifest that is being read, not component state.
+ await page.reload();
+ await expect(page.locator("[data-attrib-preview]")).toHaveText(
+ "Monday Mail #11 · 2016-09-12 @ 0:09",
+ );
+ await expect(page.locator("[data-attrib-field=title]")).toHaveValue("Monday Mail #11");
+
+ // Empty DELETES the key -- `"title": ""` in a manifest read by humans is noise
+ // that reads like a decision -- and the header goes back to the archive's.
+ await setField(page, "title", "");
+ await setField(page, "date", "");
+ await expect(page.locator("[data-attrib-preview]")).toHaveText(
+ "Fixture source vid1 · 2025-01-01 @ 0:09",
+ );
+ await expect.poll(() => readClip("c02").title).toBeUndefined();
+ await expect.poll(() => readClip("c02").date).toBeUndefined();
+});
+
+test("a date that is not a real day is refused, and the bench says so", async ({ page, request }) => {
+ // The regex alone accepts this. It reads as a date right up until somebody
+ // tries to check the clip against the stream it claims to come from.
+ const { token: t } = await token(request, "c02");
+ const res = await request.put("/api/report/window", {
+ data: { project: PROJECT, clip: "c02", date: "2025-02-31", token: t },
+ });
+ expect(res.status()).toBe(400);
+ expect(((await res.json()) as { error: string }).error).toContain("real calendar date");
+ expect(readClip("c02").date).toBeUndefined();
+
+ await page.goto(bench("c02"));
+ await setField(page, "date", "2025-02-31");
+ await expect(page.locator("[data-bench-note]")).toContainText("real calendar date");
+ // What was typed stays in the box: a rejected date is a typo to fix, and
+ // clearing it would make somebody retype the other eight characters to find
+ // out which one was wrong.
+ await expect(page.locator("[data-attrib-field=date]")).toHaveValue("2025-02-31");
+});
+
+test("prev and next walk the cut, by link and by key", async ({ page }) => {
+ await page.goto(bench("c02"));
+ await page.locator("[data-clip-nav=next]").click();
+ await expect(page.locator("[data-bench=c03]")).toBeVisible();
+
+ // The same move from the keyboard. Reviewing a cut is watching every clip in
+ // order, and the project page in between is a round trip to re-find your place.
+ await page.locator("body").press("p");
+ await expect(page.locator("[data-bench=c02]")).toBeVisible();
+
+ // The ends say so rather than offering a link into nothing.
+ await page.goto(bench("c01"));
+ await expect(page.locator("[data-clip-nav=prev]")).toHaveCount(0);
+ await page.goto(bench("c04"));
+ await expect(page.locator("[data-clip-nav=next]")).toHaveCount(0);
+});
+
+test("the segment route serves a built clip with ranges, and 404s when there is none", async ({
+ page,
+ request,
+}) => {
+ const q = (clip: string) => `project=${encodeURIComponent(PROJECT)}&clip=${clip}`;
+
+ // Nothing is rendered for c03, and that is the NORMAL state of a clip nobody
+ // has built -- the bench says so rather than showing a broken player.
+ expect((await request.get(`/api/report/segment?${q("c03")}`)).status()).toBe(404);
+ await page.goto(bench("c03"));
+ await expect(page.getByTestId("no-segment")).toBeVisible();
+ await expect(page.getByTestId("segment-video")).toHaveCount(0);
+
+ // A real segment, placed rather than built: a preview build here would be a
+ // second copy of build.spec's job, and what is being tested is the ROUTE.
+ const segDir = path.join(FIXTURE, "reports", "bench-fixture", "out", "sourced", "segments");
+ mkdirSync(segDir, { recursive: true });
+ copyFileSync(
+ path.join(FIXTURE, "reports", "bench-fixture", "out", "clips-raw", "vid1_0.00-9.00.mp4"),
+ path.join(segDir, "c01.mp4"),
+ );
+
+ const full = await request.get(`/api/report/segment?${q("c01")}`);
+ expect(full.status()).toBe(200);
+ expect(full.headers()["accept-ranges"]).toBe("bytes");
+ const mtime = full.headers()["x-segment-mtime"];
+ expect(mtime).toMatch(/^\d+$/);
+
+ // Without a 206 the <video> element will not seek in a stream it did not fully
+ // download, which is the whole interaction.
+ const part = await request.get(`/api/report/segment?${q("c01")}&v=${mtime}`, {
+ headers: { Range: "bytes=0-1023" },
+ });
+ expect(part.status()).toBe(206);
+ expect(part.headers()["content-range"]).toMatch(/^bytes 0-1023\/\d+$/);
+ // The mtime is what makes a re-render visible: a matching v may be cached
+ // hard, a stale one must not be cached at all.
+ expect(part.headers()["cache-control"]).toContain("immutable");
+ const stale = await request.get(`/api/report/segment?${q("c01")}&v=1`);
+ expect(stale.headers()["cache-control"]).toContain("no-store");
+
+ // And the bench plays it, at the URL carrying that mtime.
+ await page.goto(bench("c01"));
+ await expect(page.getByTestId("segment-video")).toHaveAttribute(
+ "src",
+ new RegExp(`api/report/segment.*v=${mtime}`),
+ );
+
+ // A name that is not this clip's is not reachable: the file name is built from
+ // the id the timeline vouched for, never from anything the client typed.
+ expect((await request.get(`/api/report/segment?${q("../../../etc/passwd")}`)).status()).toBe(404);
+});
+
+test("a correction is written for the next pass, and collected on the project page", async ({
+ page,
+}) => {
+ const text = "Speaker is Dan on a call, not Destiny, and he is talking TO Destiny.";
+ await page.goto(bench("c04"));
+ await setField(page, "correction", text);
+ await expect.poll(() => readClip("c04").correction).toBe(text);
+
+ await page.reload();
+ await expect(page.locator("[data-attrib-field=correction]")).toHaveValue(text);
+
+ // The project page lists it, because the list's real destination is somebody's
+ // next prompt -- not this manifest, which cannot fix a defect in the report.
+ await page.goto(`/browse/${PROJECT}`);
+ const list = page.locator("[data-corrections]");
+ await expect(list).toBeVisible();
+ await expect(list.locator("[data-correction=c04]")).toContainText("not Destiny");
+
+ // Empty deletes the key, and the section goes with the last correction.
+ await page.goto(bench("c04"));
+ await setField(page, "correction", "");
+ await expect.poll(() => readClip("c04").correction).toBeUndefined();
+ await page.goto(`/browse/${PROJECT}`);
+ await expect(page.locator("[data-corrections]")).toHaveCount(0);
+});
