commit 58dbf5ad2bdc141cb9a7b96ce812c2497f545e98
parent 904dc0df2ee3ba34c884edebb5e88206e8049d32
Author: I Mean I'm Just Saying <imeanimjustsaying@kiwifarms.st>
Date: Mon, 21 Sep 2026 03:31:02 -0400
review: a gate that opens on the click that opens it is not a gate
**`previewed` was set in the trigger.** The action returns as soon as the job
is enqueued, so an operator could press Preview and then Evict before a single
count had appeared. `StreamActionLog` gains `onSettled(started)` — called when
the job's stream has been read to the end, or with `false` when the trigger
refused before one existed — and the card arms on that. The age-keyed reset is
unchanged, so a preview of "older than 90 days" still cannot arm a sweep that
takes everything. The spec now asserts the button is STILL disabled between the
click and the terminal line, which is the window the bug lived in.
**`listClipWindows` still pre-filtered on `.mp4`.** With the parser widened but
the lister not, a `.mkv` window was invisible to the video page and to
`findContainingClipWindow` — the fetch dedup, which would then have paid a
source for those seconds again — while `evictClipWindows` could see and delete
it. A file one half of the code owns and the other cannot see is the worst of
both. One predicate now (`hasClipWindowExt`, beside the extension list the
parser strips), and a `.json` sidecar still has none of those extensions, so it
is read BY a window and never listed AS one.
common/lib/clipWindow-server.test.ts is new: a `.webm` window lists and is found
by `findContainingClipWindow`, all three extensions list and nothing else does,
and a video dir with no `clips/` lists nothing rather than throwing.
Co-Authored-By: Claude Fable 5.1 <noreply@anthropic.com>
Diffstat:
6 files changed, 140 insertions(+), 9 deletions(-)
diff --git a/common/components/StreamActionLog.tsx b/common/components/StreamActionLog.tsx
@@ -26,6 +26,17 @@ type Props = {
* that need a typed-confirmation gate before allowing the action.
*/
disabled?: boolean;
+ /**
+ * Called when a run SETTLES — the job's stream has been read to the end (or
+ * the trigger refused before one existed). `started` is false for a refusal.
+ *
+ * WHY THIS IS NOT "ON CLICK". A caller gating a destructive button behind a
+ * dry run has to arm it when the dry run has ANSWERED, not when it was
+ * asked: the trigger returns as soon as the job is enqueued, so flipping a
+ * flag there lets an operator press Preview and then Evict before a single
+ * count has appeared — which is the whole thing the gate exists to stop.
+ */
+ onSettled?: (started: boolean) => void;
};
type StatusPoll = {
@@ -42,6 +53,7 @@ export function StreamActionLog({
cancelAction,
extraControls,
disabled = false,
+ onSettled,
}: Props) {
const accessibleName = label ?? buttonLabel;
const router = useRouter();
@@ -154,6 +166,7 @@ export function StreamActionLog({
setError((e as Error).message);
} finally {
setRunning(false);
+ onSettled?.(started);
// A run that actually started (success, mid-stream error, or cancel)
// may have mutated server-derived data: a saved shard config, channel
// counts, bucket lists, failed-transcription lists. Re-fetch the server
diff --git a/common/lib/clipWindow-server.test.ts b/common/lib/clipWindow-server.test.ts
@@ -0,0 +1,91 @@
+import { test } from "node:test";
+import assert from "node:assert/strict";
+import { mkdir, mkdtemp, rm, writeFile } from "node:fs/promises";
+import { tmpdir } from "node:os";
+import path from "node:path";
+import { findContainingClipWindow, listClipWindows } from "./clipWindow-server";
+
+// THE LISTER AND THE EVICTOR MUST AGREE ABOUT WHAT A WINDOW IS.
+//
+// They did not: `listClipWindows` pre-filtered on `.mp4` while
+// `parseClipWindowName` had been widened, so a `.mkv` window was invisible to
+// the video page and to `findContainingClipWindow` — the fetch dedup, which
+// would then have paid a source for those seconds again — while
+// `evictClipWindows` could see and delete it. A file one half of the code owns
+// and the other cannot see is the worst of both.
+
+async function withClips(
+ files: Record<string, string>,
+ fn: (videoDir: string) => Promise<void>,
+): Promise<void> {
+ const dir = await mkdtemp(path.join(tmpdir(), "ttb-clipwin-"));
+ try {
+ const clips = path.join(dir, "clips");
+ await mkdir(clips, { recursive: true });
+ for (const [name, body] of Object.entries(files)) {
+ await writeFile(path.join(clips, name), body);
+ }
+ await fn(dir);
+ } finally {
+ await rm(dir, { recursive: true, force: true });
+ }
+}
+
+test("a .webm window lists, and the fetch dedup finds it", async () => {
+ await withClips(
+ {
+ "10.00-40.00.webm": "not really webm",
+ "10.00-40.00.json": JSON.stringify({ requestedBy: "umtool" }),
+ },
+ async (videoDir) => {
+ const windows = await listClipWindows(videoDir);
+ assert.deepEqual(
+ windows.map((w) => w.file),
+ ["10.00-40.00.webm"],
+ );
+ assert.equal(windows[0].from, 10);
+ assert.equal(windows[0].to, 40);
+ // The sidecar was read BY the window, never listed AS one.
+ assert.equal(windows[0].provenance?.requestedBy, "umtool");
+
+ const found = await findContainingClipWindow(videoDir, 12, 30);
+ assert.equal(found?.file, "10.00-40.00.webm");
+ // And a span it does not hold is still a miss.
+ assert.equal(await findContainingClipWindow(videoDir, 12, 90), null);
+ },
+ );
+});
+
+test("all three extensions list, and nothing else does", async () => {
+ await withClips(
+ {
+ "1.00-2.00.mp4": "a",
+ "3.00-4.00.mkv": "b",
+ "5.00-6.00.webm": "c",
+ // A sidecar with no window: it has no media extension, so it is not a
+ // window — and listing it would put a file with no bytes of media in
+ // front of the operator as one.
+ "7.00-8.00.json": "{}",
+ // Not a `<from>-<to>` name at all.
+ "notes.txt": "d",
+ "audio.mp4": "e",
+ },
+ async (videoDir) => {
+ const windows = await listClipWindows(videoDir);
+ assert.deepEqual(
+ windows.map((w) => w.file).sort(),
+ ["1.00-2.00.mp4", "3.00-4.00.mkv", "5.00-6.00.webm"],
+ );
+ },
+ );
+});
+
+test("a video dir with no clips/ lists nothing rather than throwing", async () => {
+ const dir = await mkdtemp(path.join(tmpdir(), "ttb-clipwin-"));
+ try {
+ assert.deepEqual(await listClipWindows(dir), []);
+ assert.equal(await findContainingClipWindow(dir, 1, 2), null);
+ } finally {
+ await rm(dir, { recursive: true, force: true });
+ }
+});
diff --git a/common/lib/clipWindow-server.ts b/common/lib/clipWindow-server.ts
@@ -9,6 +9,7 @@ import {
clipWindowFile,
clipWindowSidecar,
parseClipProvenance,
+ hasClipWindowExt,
parseClipWindowName,
tightestClipWindow,
type ClipProvenance,
@@ -36,7 +37,10 @@ export async function listClipWindows(videoDir: string): Promise<ClipWindow[]> {
const entries = await readdir(dir).catch(() => [] as string[]);
const out: ClipWindow[] = [];
for (const name of entries) {
- if (!name.endsWith(".mp4")) continue;
+ // THE SHARED PREDICATE, not a second copy of the extension list — see
+ // `hasClipWindowExt`. A `.json` sidecar has none of these extensions, so it
+ // never lists as a window; it is read BY the window it describes.
+ if (!hasClipWindowExt(name)) continue;
const span = parseClipWindowName(name);
if (!span) continue;
const abs = path.join(dir, name);
diff --git a/common/lib/clipWindow.ts b/common/lib/clipWindow.ts
@@ -85,10 +85,20 @@ const WINDOW_RE = /^(\d+(?:\.\d+)?)-(\d+(?:\.\d+)?)$/;
//
// NOT `.json`: a sidecar is removed with the window it describes and must
// never parse as one itself.
-const CLIP_EXTS = [".mp4", ".mkv", ".webm"] as const;
+export const CLIP_WINDOW_EXTS = [".mp4", ".mkv", ".webm"] as const;
+
+// ONE PREDICATE, so the lister and the evictor cannot disagree about what a
+// window is. They did: `listClipWindows` pre-filtered on `.mp4` while the
+// parser had been widened, so a `.mkv` window was invisible to the video page
+// and to `findContainingClipWindow` (the fetch dedup — it would have paid for
+// those seconds again) while `evictClipWindows` could see and delete it. A file
+// one half of the code owns and the other cannot see is the worst of both.
+export function hasClipWindowExt(name: string): boolean {
+ return CLIP_WINDOW_EXTS.some((ext) => name.endsWith(ext));
+}
function stripClipExt(name: string): string {
- for (const ext of CLIP_EXTS) {
+ for (const ext of CLIP_WINDOW_EXTS) {
if (name.endsWith(ext)) return name.slice(0, -ext.length);
}
return name;
diff --git a/editor/app/storage/components/ClipWindowsCard.tsx b/editor/app/storage/components/ClipWindowsCard.tsx
@@ -30,11 +30,16 @@ import { evictClipWindowsAction } from "../actions";
// PREVIEW FIRST, AND THE PREVIEW IS THE GATE. `dryRun` runs the identical pass
// and deletes nothing, so the number in the log is produced by the code that
// would do the work rather than by a second estimate that can disagree — and
-// until one has run in this session the destructive button is disabled. One
-// click should not be able to delete every clip window in the corpus, and
+// until one has ANSWERED in this session the destructive button is disabled.
+// One click should not be able to delete every clip window in the corpus, and
// "older than 30 days" over a corpus nobody has looked at is a number the
// operator has no way to picture.
//
+// ARMED ON `onSettled`, NEVER IN THE TRIGGER. The trigger returns as soon as
+// the job is enqueued, so flipping the flag there would let somebody press
+// Preview and then Evict before a single count had appeared — a gate that
+// unlocks on the click that opens it is not a gate.
+//
// "ANY AGE" IS A SECOND GATE. `0` means every window on every drive, which is
// a legitimate ask (the operator is emptying a disk) and the one setting where
// a preview alone is not enough of a pause. It needs the checkbox ticked as
@@ -129,9 +134,13 @@ export function ClipWindowsCard({ clipsBytes }: { clipsBytes: number }) {
<div className="flex flex-wrap items-start gap-4">
<StreamActionLog
key="evict-clips-preview-log"
- trigger={() => {
- setPreviewed(true);
- return evictClipWindowsAction({ olderThanDays: days, dryRun: true });
+ trigger={() =>
+ evictClipWindowsAction({ olderThanDays: days, dryRun: true })
+ }
+ // `started` is false when the action refused before a job existed —
+ // a refusal is not a preview, so it arms nothing.
+ onSettled={(started) => {
+ if (started) setPreviewed(true);
}}
cancelAction={cancelJobAction}
buttonLabel="Preview eviction"
@@ -152,7 +161,7 @@ export function ClipWindowsCard({ clipsBytes }: { clipsBytes: number }) {
<p aria-label="clip eviction gate" className="text-xs text-muted-foreground">
{previewed
? "Tick the box above to evict every window."
- : "Preview first — the eviction button unlocks once you have seen what it would take."}
+ : "Preview first — the eviction button unlocks once the dry run has reported what it would take."}
</p>
)}
</article>
diff --git a/editor/e2e/storage-locations.spec.ts b/editor/e2e/storage-locations.spec.ts
@@ -660,7 +660,11 @@ test("evicting at any age needs the tick as well as the preview", async ({
await confirm.check();
await expect(evict).toBeDisabled();
+ // ARMED BY THE ANSWER, NOT BY THE CLICK. The action returns as soon as the
+ // job is enqueued, so a gate flipped in the trigger would already be open
+ // here — which is the whole thing it exists to stop.
await page.getByRole("button", { name: "Preview eviction" }).click();
+ await expect(evict).toBeDisabled();
await expect(page.getByLabel("Preview eviction output")).toContainText(
/Would evict/,
{ timeout: 60_000 },
