fix(downloads): GIF review round 2 — cap as sent, shown length, retry, Join reason

The panels judge a GIF's length in whole milliseconds between the ends as sent, the server's own rule, and a refused length reads rounded up. A shared picture says it failed only once its retry failed too. Join's reason shows on a key's focus; the editor's failure toasts name GIFs. The edit route's cap is test-held, and DESIGN.md records the GIF UI.
This commit is contained in:
2026-11-13 00:19:53 +01:00
parent 8845d1aa49
commit fe6af1445f
12 changed files with 200 additions and 43 deletions
+8 -2
View File
@@ -12,7 +12,8 @@ import { useEffect, useState, type ImgHTMLAttributes, type ReactNode } from "rea
// `r` parameter (measured 2026-10-14). A 429 from a burst of cards heals this way.
// - While there is no picture — none given, a retry still waiting, every retry spent — the caller's
// `fallback` stands in (nothing by default: the card's own empty frame, which is what a card with no
// thumbnail has always shown). Never the broken-image mark.
// thumbnail has always shown), or its `pending` while a retry waits when the fallback says the
// picture failed. Never the broken-image mark.
// - The failure is remembered BY URL, in render: these sit in virtualized grids whose slots are recycled,
// and a failure stored as a flag an effect resets painted one item's fallback over the next one's
// picture for a frame (ui/Avatar had that defect).
@@ -47,6 +48,7 @@ export default function RemoteImage({
src,
srcSet,
fallback = null,
pending = fallback,
retries = IMAGE_RETRIES,
...img
}: Omit<ImgHTMLAttributes<HTMLImageElement>, "src" | "srcSet" | "onError" | "referrerPolicy"> & {
@@ -54,6 +56,9 @@ export default function RemoteImage({
srcSet?: string;
/** What stands in while there is no picture. */
fallback?: ReactNode;
/** What stands in while a retry waits, when that is not the fallback: a fallback that says the
* picture failed would say it of one that may still come. */
pending?: ReactNode;
/** Fewer retries for a picture whose failure is an answer, not a hiccup — a file that is not there
* yet is better shown as its fallback now than after seven seconds of retries. */
retries?: number;
@@ -75,7 +80,8 @@ export default function RemoteImage({
}, [waiting, attempt, src]);
const spent = current !== null && !current.waiting && current.attempt > retries;
if (!src || waiting || spent) return <>{fallback}</>;
if (waiting && src) return <>{pending}</>;
if (!src || spent) return <>{fallback}</>;
return (
// eslint-disable-next-line no-restricted-syntax -- the one <img> the rest of the app goes through
<img
+8 -4
View File
@@ -12,8 +12,10 @@ import { log } from "../../lib/log";
import { formatTimecode } from "../../lib/timecode";
import { formatNumber } from "../../lib/format";
import {
atMs,
estimateGif,
gifEstimateText,
gifShownLength,
gifVerdict,
GIF_DEFAULT_SIDE,
GIF_MAX_S,
@@ -146,7 +148,7 @@ export default function ClipPanel({
// how many pixels it holds.
const shape = crop ? (crop.rect.w * picture) / crop.rect.h : picture;
const estimate = estimateGif(length, side, shape);
const verdict = gif ? gifVerdict(length, estimate) : "ok";
const verdict = gif ? gifVerdict(clip.start, clip.end, estimate) : "ok";
const estimateText = gifEstimateText(estimate, i18n.language);
const blocked = verdict === "too_long";
@@ -197,8 +199,8 @@ export default function ClipPanel({
const body: ClipRequest = {
source: src.source,
ref: src.ref,
start_s: Math.round(clip.start * 1000) / 1000,
end_s: Math.round(clip.end * 1000) / 1000,
start_s: atMs(clip.start),
end_s: atMs(clip.end),
...(src.audioTrack != null ? { audio_track: src.audioTrack } : {}),
...(crop
? {
@@ -347,7 +349,9 @@ export default function ClipPanel({
onHere={() => moveOut(current, true)}
/>
<span className="tabular-nums text-muted" aria-live="polite">
{t("clip.length", { length: formatTimecode(length) })}
{t("clip.length", {
length: formatTimecode(gif ? gifShownLength(clip.start, clip.end) : length),
})}
</span>
<Button
variant="overlay"
+2
View File
@@ -24,6 +24,8 @@
"created_other": "{{count}} clips added to your downloads",
"failed": "Couldn’t create the clip",
"partlyCreated": "Only {{done}} of {{total}} clips were added — the rest are still selected, so creating again adds just those.",
"failedGif": "Couldn’t create the GIF",
"partlyCreatedGif": "Only {{done}} of {{total}} GIFs were added — the rest are still selected, so creating again adds just those.",
"splitHere": "Split here",
"output": {
"label": "Output",
+2
View File
@@ -24,6 +24,8 @@
"created_other": "{{count}} klip a letöltésekhez adva",
"failed": "Nem sikerült létrehozni a klipet",
"partlyCreated": "{{total}} klipből csak {{done}} került a letöltésekhez — a többi kijelölve maradt, így az újbóli létrehozás csak azokat adja hozzá.",
"failedGif": "Nem sikerült létrehozni a GIF-et",
"partlyCreatedGif": "{{total}} GIF-ből csak {{done}} került a letöltésekhez — a többi kijelölve maradt, így az újbóli létrehozás csak azokat adja hozzá.",
"splitHere": "Vágás itt",
"output": {
"label": "Kimenet",
+30 -8
View File
@@ -4,8 +4,10 @@ import {
gifEstimateText,
GIF_MAX_BYTES,
GIF_PREVIEW_MAX_BYTES,
gifShownLength,
gifVerdict,
} from "./gif";
import { formatTimecode } from "./timecode";
const MB = 1024 * 1024;
@@ -50,22 +52,42 @@ describe("gifEstimateText", () => {
describe("gifVerdict", () => {
it("refuses past the length cap, whatever the size", () => {
expect(gifVerdict(30.2, { hi: 1 })).toBe("too_long");
expect(gifVerdict(30, { hi: 1 })).toBe("ok");
expect(gifVerdict(0, 30.2, { hi: 1 })).toBe("too_long");
expect(gifVerdict(0, 30, { hi: 1 })).toBe("ok");
});
it("judges the length the server judges: whole milliseconds between the ends as sent", () => {
// A float difference a hair over 30 s is exactly 30 s once the ends are sent.
expect(60.529 - 30.529).toBeGreaterThan(30);
expect(gifVerdict(30.529, 60.529, { hi: 1 })).toBe("ok");
expect(gifVerdict(0, 30.001, { hi: 1 })).toBe("too_long");
// Each end is sent rounded on its own: 10.0004–40.0008 goes as 10–40.001, 30001 ms, refused…
expect(gifVerdict(10.0004, 40.0008, { hi: 1 })).toBe("too_long");
// …and 10.0006–40.0012 as 10.001–40.001, exactly 30 s, made.
expect(gifVerdict(10.0006, 40.0012, { hi: 1 })).toBe("ok");
});
it("warns past the server's cap, then past the link preview's", () => {
expect(gifVerdict(10, { hi: GIF_MAX_BYTES + 0.6 * MB })).toBe("may_fail");
expect(gifVerdict(10, { hi: GIF_MAX_BYTES })).toBe("no_preview");
expect(gifVerdict(10, { hi: GIF_PREVIEW_MAX_BYTES + 0.6 * MB })).toBe("no_preview");
expect(gifVerdict(10, { hi: GIF_PREVIEW_MAX_BYTES })).toBe("ok");
expect(gifVerdict(0, 10, { hi: GIF_MAX_BYTES + 0.6 * MB })).toBe("may_fail");
expect(gifVerdict(0, 10, { hi: GIF_MAX_BYTES })).toBe("no_preview");
expect(gifVerdict(0, 10, { hi: GIF_PREVIEW_MAX_BYTES + 0.6 * MB })).toBe("no_preview");
expect(gifVerdict(0, 10, { hi: GIF_PREVIEW_MAX_BYTES })).toBe("ok");
});
it("judges the size the reader is shown: a high end shown as 30 MB is not past 30 MB", () => {
// 30 s at 480 lands a hair over 30 MiB, and reads "about 12–30 MB".
const hi = GIF_MAX_BYTES + 0.4 * MB;
expect(gifEstimateText({ lo: 12 * MB, hi }, "en").hi).toBe("30");
expect(gifVerdict(30, { hi })).toBe("no_preview");
expect(gifVerdict(10, { hi: GIF_PREVIEW_MAX_BYTES + 0.4 * MB })).toBe("ok");
expect(gifVerdict(0, 30, { hi })).toBe("no_preview");
expect(gifVerdict(0, 10, { hi: GIF_PREVIEW_MAX_BYTES + 0.4 * MB })).toBe("ok");
});
});
describe("gifShownLength", () => {
it("never shows a refused range as one the cap allows", () => {
expect(formatTimecode(gifShownLength(0, 30.03))).toBe("0:30.1");
expect(formatTimecode(gifShownLength(0, 30.001))).toBe("0:30.1");
expect(gifShownLength(30.529, 60.529)).toBe(30);
expect(gifShownLength(0, 12.34)).toBe(12.34);
});
});
+18 -2
View File
@@ -53,8 +53,24 @@ export function gifEstimateText(est: { lo: number; hi: number }, lang: string) {
* pass the cap (refused when it does), it may be too big to move in a link preview, or fine. */
export type GifVerdict = "too_long" | "may_fail" | "no_preview" | "ok";
export function gifVerdict(seconds: number, est: { hi: number }): GifVerdict {
if (seconds > GIF_MAX_S + 0.0005) return "too_long";
/** A time as the request sends it: to the millisecond. */
export const atMs = (s: number) => Math.round(s * 1000) / 1000;
/** A range's length in whole milliseconds, between its ends as the request sends them: the length
* the server judges (gif.too_long), so a range the panel allows is never refused for its length,
* and none it refuses would have been made. */
export const spanMs = (start: number, end: number) =>
Math.round(end * 1000) - Math.round(start * 1000);
/** A range's length to show beside a verdict: over the cap it is rounded up to the tenth, so a
* refused range never reads as one the cap allows ("0:30.0" for 30.03 s). */
export function gifShownLength(start: number, end: number): number {
const ms = spanMs(start, end);
return ms > GIF_MAX_S * 1000 ? Math.ceil(ms / 100) / 10 : ms / 1000;
}
export function gifVerdict(start: number, end: number, est: { hi: number }): GifVerdict {
if (spanMs(start, end) > GIF_MAX_S * 1000) return "too_long";
// Judged on the high end as shown (whole megabytes): "about 12–30 MB" beside "it may pass 30 MB"
// would contradict itself, and the estimate is no finer than that anyway.
const hi = Math.round(est.hi / MB) * MB;
@@ -9,9 +9,15 @@ import { QueryClient, QueryClientProvider } from "@tanstack/react-query";
vi.mock("react-i18next", async (importOriginal) => ({
...(await importOriginal<typeof import("react-i18next")>()),
// A size option reads as its own words with its unit ("480 px"); every other text is its key.
// A size option reads as its own words with its unit ("480 px"), a too-long note with the length
// it gives; every other text is its key.
useTranslation: () => ({
t: (key: string, o?: { n?: string }) => (key === "clip.gif.side" ? `${o?.n} px` : key),
t: (key: string, o?: { n?: string; length?: string }) =>
key === "clip.gif.side"
? `${o?.n} px`
: key === "editor.format.segTooLong"
? `${key} ${o?.length}`
: key,
i18n: { language: "en" },
}),
}));
@@ -22,9 +28,16 @@ vi.mock("../../lib/notifications", async (importOriginal) => ({
}));
import VideoEditor from "./VideoEditor";
import { api, type DownloadJob } from "../../lib/api";
import { api, HttpError, type DownloadJob } from "../../lib/api";
import { markReported } from "../../lib/api/core";
import { msg } from "../../lib/notifications";
// A refusal as `req` delivers it: already shown in its error dialog, and marked so.
const markedReported = (e: HttpError) => {
markReported(e);
return e;
};
beforeAll(() => {
class Stub {
observe() {}
@@ -94,7 +107,7 @@ describe("VideoEditor — GIF", () => {
fireEvent.click(screen.getByRole("button", { name: "editor.format.gif" }));
const join = screen.getByRole("button", { name: "editor.output.join" });
expect(join.getAttribute("aria-disabled")).toBe("true");
expect(join.getAttribute("title")).toBe("editor.output.joinGif");
expect(join.getAttribute("aria-description")).toBe("editor.output.joinGif");
expect(join.getAttribute("aria-pressed")).toBe("false");
fireEvent.click(screen.getByRole("button", { name: "editor.createGif" }));
await waitFor(() => expect(enqueue).toHaveBeenCalledTimes(2));
@@ -110,7 +123,7 @@ describe("VideoEditor — GIF", () => {
const create = screen.getByRole("button", { name: "editor.createGif" });
expect(create.getAttribute("aria-disabled")).toBe("true");
const notes = document.getElementById(create.getAttribute("aria-describedby")!)!;
expect(notes.textContent).toBe("editor.format.segTooLong");
expect(notes.textContent).toBe("editor.format.segTooLong 1:40.0");
fireEvent.click(create);
expect(enqueue).not.toHaveBeenCalled();
// A video of the same range has no such limit.
@@ -120,6 +133,43 @@ describe("VideoEditor — GIF", () => {
).toBe(null);
});
it("says a refused length so it never reads as one the cap allows", () => {
// A phone video of 30.03 s: refused, and said as 0:30.1, not 0:30.0.
open(30.03);
fireEvent.click(screen.getByRole("button", { name: "editor.format.gif" }));
const create = screen.getByRole("button", { name: "editor.createGif" });
expect(create.getAttribute("aria-disabled")).toBe("true");
const notes = document.getElementById(create.getAttribute("aria-describedby")!)!;
expect(notes.textContent).toBe("editor.format.segTooLong 0:30.1");
});
it("says a GIF failure as one of GIFs, not of clips", async () => {
const { enqueue, playhead } = open(20);
enqueue
.mockReset()
.mockResolvedValueOnce({} as never)
.mockRejectedValueOnce(
markedReported(new HttpError(422, "errors.generic", { code: "quota" })),
)
.mockRejectedValueOnce(new Error("offline"));
playhead(8);
fireEvent.click(screen.getByText("editor.splitHere"));
fireEvent.click(screen.getByRole("button", { name: "editor.format.gif" }));
fireEvent.click(screen.getByRole("button", { name: "editor.createGif" }));
await waitFor(() =>
expect(notify).toHaveBeenCalledWith(
expect.objectContaining({ title: msg("editor.partlyCreatedGif", { done: 1, total: 2 }) }),
),
);
// The one left fails on its own: said as a GIF too.
fireEvent.click(screen.getByRole("button", { name: "editor.createGif" }));
await waitFor(() =>
expect(notify).toHaveBeenCalledWith(
expect.objectContaining({ title: msg("editor.failedGif") }),
),
);
});
it("offers no GIF for an audio file, which has no picture to make one of", () => {
open(20, "audio");
expect(screen.queryByRole("button", { name: "editor.format.gif" })).toBeNull();
@@ -151,11 +201,15 @@ describe("VideoEditor — GIF", () => {
expect(screen.queryByText("editor.format.estimate")).toBeNull();
});
it("Join needs two kept segments, and says so instead of going dead", () => {
it("Join needs two kept segments, and says so instead of going dead", async () => {
open(20);
const join = screen.getByRole("button", { name: "editor.output.join" });
expect(join.hasAttribute("disabled")).toBe(false);
expect(join.getAttribute("aria-disabled")).toBe("true");
expect(join.getAttribute("title")).toBe("editor.output.joinNeedsTwo");
expect(join.getAttribute("aria-description")).toBe("editor.output.joinNeedsTwo");
// The reason is seen on a key's focus too, not only under a mouse (a native title is not).
expect(join.hasAttribute("title")).toBe(false);
fireEvent.focus(join);
expect(await screen.findByText("editor.output.joinNeedsTwo")).toBeTruthy();
});
});
+27 -19
View File
@@ -12,6 +12,8 @@ import { cropToSourcePx, type Frac } from "../../lib/cropGeometry";
import { CropFrame } from "../../components/ui/CropFrame";
import SegmentedToggle from "../../components/ui/SegmentedToggle";
import { Button } from "../../components/ui/Button";
import Tooltip from "../../components/ui/Tooltip";
import { unavailable } from "../../components/ui/unavailable";
import { failureReason, notifyActionError } from "../../lib/actionErrors";
import { markReported } from "../../lib/api/core";
import { IconButton } from "../../components/ui/IconButton";
@@ -25,8 +27,10 @@ import { trackLeft } from "../../components/ui/trackPosition";
import { formatTimecode } from "../../lib/timecode";
import { formatNumber } from "../../lib/format";
import {
atMs,
estimateGif,
gifEstimateText,
gifShownLength,
gifVerdict,
GIF_DEFAULT_SIDE,
GIF_MAX_S,
@@ -263,9 +267,9 @@ export default function VideoEditor({ job, onClose }: { job: DownloadJob; onClos
const est = estimateGif(s.end - s.start, side, gifShape);
return {
n: segments.indexOf(s) + 1,
length: s.end - s.start,
length: gifShownLength(s.start, s.end),
est,
verdict: gifVerdict(s.end - s.start, est),
verdict: gifVerdict(s.start, s.end, est),
};
});
const gifTotal = gifSegs.reduce((a, g) => ({ lo: a.lo + g.est.lo, hi: a.hi + g.est.hi }), {
@@ -300,7 +304,7 @@ export default function VideoEditor({ job, onClose }: { job: DownloadJob; onClos
const base = name.trim() || defaultName;
if (willJoin) {
const spec: EditSpec = {
segments: kept.map((s) => ({ start_s: +s.start.toFixed(3), end_s: +s.end.toFixed(3) })),
segments: kept.map((s) => ({ start_s: atMs(s.start), end_s: atMs(s.end) })),
accurate,
};
if (cp) spec.crop = cp;
@@ -322,7 +326,7 @@ export default function VideoEditor({ job, onClose }: { job: DownloadJob; onClos
// cannot lower a conditional or a loop inside one yet, and skipped the whole editor for it.
const clips = kept.map((s) => {
const spec: EditSpec = {
trim: { start_s: +s.start.toFixed(3), end_s: +s.end.toFixed(3) },
trim: { start_s: atMs(s.start), end_s: atMs(s.end) },
};
if (cp) spec.crop = cp;
if (gif) spec.gif = { side };
@@ -346,7 +350,10 @@ export default function VideoEditor({ job, onClose }: { job: DownloadJob; onClos
// what the dialog does not tell.
notify({
level: "error",
title: msg("editor.partlyCreated", { done: done.length, total: N }),
title: msg(gif ? "editor.partlyCreatedGif" : "editor.partlyCreated", {
done: done.length,
total: N,
}),
message: failureReason(e),
});
markReported(e);
@@ -364,7 +371,7 @@ export default function VideoEditor({ job, onClose }: { job: DownloadJob; onClos
});
onClose();
},
onError: (e) => notifyActionError(e, msg("editor.failed")),
onError: (e) => notifyActionError(e, msg(gif ? "editor.failedGif" : "editor.failed")),
onSettled: () => {
creating.current = false;
},
@@ -607,20 +614,21 @@ export default function VideoEditor({ job, onClose }: { job: DownloadJob; onClos
<span className="text-muted">{t("editor.output.label")}:</span>
{(["separate", "join"] as const).map((m) => {
// Join is unavailable with its reason in its tooltip (a GIF is never joined; a join
// needs two kept segments), and stays focusable so the reason reaches the keyboard.
const unavailable = m === "join" ? joinReason : undefined;
// needs two kept segments), shown on focus too, and stays focusable so the reason
// reaches the keyboard.
const reason = m === "join" ? joinReason : undefined;
return (
<Button
key={m}
variant="outline"
size="sm"
pressed={(willJoin ? "join" : "separate") === m}
onClick={() => setOutput(m)}
aria-disabled={unavailable ? true : undefined}
title={unavailable}
>
{t(`editor.output.${m}`)}
</Button>
<Tooltip key={m} hint={reason ?? ""} always={!!reason} stable>
<Button
variant="outline"
size="sm"
pressed={(willJoin ? "join" : "separate") === m}
onClick={() => setOutput(m)}
{...unavailable(!!reason, reason)}
>
{t(`editor.output.${m}`)}
</Button>
</Tooltip>
);
})}
<div className="flex-1" />
@@ -197,6 +197,12 @@ describe("WatchPage media element", () => {
});
const page = render(<WatchPage />);
fireEvent.error(await page.findByRole("img", { name: "Gone" }));
// The first miss may be a hiccup: nothing is said while the retry waits…
expect(page.queryByRole("status")).toBeNull();
// …and when the retry fails too, the page says so.
const retry = await page.findByRole("img", { name: "Gone" }, { timeout: 3000 });
expect(retry.getAttribute("src")).toBe("/gone.gif?r=1");
fireEvent.error(retry);
expect((await page.findByRole("status")).textContent).toBe("Couldn't load the picture.");
});
+3 -1
View File
@@ -489,7 +489,9 @@ function SharedPicture({ meta, nested }: { meta: FileMeta; nested: boolean }) {
height={meta.height ?? undefined}
retries={1}
// A picture link always has a picture: a missing one failed to load, and says so —
// the bare file glyph would read as a file with no preview.
// the bare file glyph would read as a file with no preview. Only once the retry failed
// too: while it waits, the frame stays empty.
pending={<div aria-hidden className="w-48 h-32" />}
fallback={
<div role="status" className="flex flex-col items-center gap-2 px-12 py-10 text-muted">
<FileIcon className="w-12 h-12" aria-hidden />