From 2768ab5bdd2c6db5924688f8a9612a2b7d1b12e8 Mon Sep 17 00:00:00 2001 From: npeter83 Date: Mon, 5 Oct 2026 03:16:13 +0200 Subject: [PATCH 01/12] fix(gate): the plan-code guard catches decision codes, and the purge takes them A plan's decision ids (`D1`, `D4`, `D6-A`) cited in 11 comments and test titles named a document no reader of this repo has, and the guard had no pattern for them. It has now (mirrored in audit_metrics), with cases for each form the citations took and for the look-alikes it must leave alone (a JPEG marker, "3D"); the 11 lines say their why without the id. Case test seen red without the pattern, the scan red on a planted `D9:`. --- backend/app/downloads/edit.py | 2 +- backend/app/routes/downloads.py | 2 +- backend/app/sync/tagsuggest.py | 2 +- backend/tests/test_links.py | 2 +- frontend/src/components/PlexBrowse.tsx | 4 ++-- frontend/src/components/PlexDiscover.dom.test.tsx | 2 +- frontend/src/components/ShareDialog.tsx | 4 ++-- frontend/src/components/UpNextCard.tsx | 2 +- frontend/src/components/plexCards.dom.test.tsx | 2 +- frontend/src/lib/keymap.sequence.test.ts | 4 ++-- frontend/src/lib/planCodes.contract.test.ts | 13 +++++++++---- scripts/audit_metrics.py | 9 +++++---- 12 files changed, 27 insertions(+), 21 deletions(-) diff --git a/backend/app/downloads/edit.py b/backend/app/downloads/edit.py index 24498872..d191d14a 100644 --- a/backend/app/downloads/edit.py +++ b/backend/app/downloads/edit.py @@ -734,7 +734,7 @@ def _step_failure(err: BaseException) -> str: def _is_jpeg(path: Path) -> bool: - """True if the file starts with the JPEG SOI marker (FF D8 FF).""" + """True if the file starts with the JPEG SOI marker (0xFF 0xD8 0xFF).""" try: with path.open("rb") as f: return f.read(3) == b"\xff\xd8\xff" diff --git a/backend/app/routes/downloads.py b/backend/app/routes/downloads.py index cb9e3e23..d2e1c5d2 100644 --- a/backend/app/routes/downloads.py +++ b/backend/app/routes/downloads.py @@ -1033,7 +1033,7 @@ def list_links( class CreateLinkIn(BaseModel): password: str = "" - # D1: a newly shared download is downloadable by default — the common intent is "let the + # A newly shared download is downloadable by default — the common intent is "let the # recipient have the file", not stream-only. Forward-only: this only sets the default for links # minted from now on; existing links keep whatever `allow_download` they were created with (the # column default stays False, so no back-catalogue link silently flips). The owner can still diff --git a/backend/app/sync/tagsuggest.py b/backend/app/sync/tagsuggest.py index 14d0f7e6..a4e2d99e 100644 --- a/backend/app/sync/tagsuggest.py +++ b/backend/app/sync/tagsuggest.py @@ -12,7 +12,7 @@ result (measured on real data: every untagged channel got a suggestion, most on alone). Topic overlap is what makes "you tagged similar channels this way" meaningful. Purely read-only and deterministic — the Channel Manager surfaces the result as one-tap-accept -"ghost" chips; nothing is written here (D4 = suggest-and-confirm, no silent auto-apply). The +"ghost" chips; nothing is written here (suggest-and-confirm, never a silent auto-apply). The thresholds are tuning constants rather than admin knobs on purpose: they mirror how ``autotag.py`` keeps its mappings inline, and sysconfig only stores int/str/bool (no float), so a knob would mean storing percentages — not worth the surface for values an operator has no reason to tune per-install. diff --git a/backend/tests/test_links.py b/backend/tests/test_links.py index 78c18096..f3e8e47d 100644 --- a/backend/tests/test_links.py +++ b/backend/tests/test_links.py @@ -99,7 +99,7 @@ class TestPublicMetaAudioFlag: class TestCreateLinkDefault: - """D1: a newly minted share link is downloadable by default. The one-click Share relies on + """A newly minted share link is downloadable by default. The one-click Share relies on this — it POSTs no `allow_download`, so the API default decides. Forward-only lives here (the API default), NOT in a migration: the DownloadLink column default stays False, so no existing back- catalogue link silently flips to downloadable.""" diff --git a/frontend/src/components/PlexBrowse.tsx b/frontend/src/components/PlexBrowse.tsx index c65a1cf7..d8f9f0a6 100644 --- a/frontend/src/components/PlexBrowse.tsx +++ b/frontend/src/components/PlexBrowse.tsx @@ -68,7 +68,7 @@ const PlexCollectionView = lazy(() => import("./PlexCollectionView")); const PAGE = 40; -// D3: the "last watched" sort auto-applied when you enter the in-progress watch-state view (most +// The "last watched" sort auto-applied when you enter the in-progress watch-state view (most // recently watched first). Frozen for the same reason as PLEX_SORT_FALLBACK (handed out by reference). const LASTWATCHED_SORT: NonNullable = Object.freeze({ key: "lastwatched", @@ -229,7 +229,7 @@ export default function PlexBrowse() { return () => window.removeEventListener("keydown", onKey); }, [sub, canStepBack]); - // D3: entering the in-progress watch-state view auto-applies the "last watched" sort (most recently + // Entering the in-progress watch-state view auto-applies the "last watched" sort (most recently // watched first); leaving restores the sort that was active before entering. Only the ENTER edge // forces the sort, so a manual sort change while in the in-progress view still wins — it's just not // carried back out (exit returns to the pre-entry preference). We stash the raw provider `sort`, not diff --git a/frontend/src/components/PlexDiscover.dom.test.tsx b/frontend/src/components/PlexDiscover.dom.test.tsx index 63c4ace6..4dd6537a 100644 --- a/frontend/src/components/PlexDiscover.dom.test.tsx +++ b/frontend/src/components/PlexDiscover.dom.test.tsx @@ -273,7 +273,7 @@ describe("PlexDiscover", () => { }); // Every rail and the genre grid are lists the keyboard walks (lib/listNav), so `j` goes on -// from one to the next in page order (owner decision D4). +// from one to the next in page order (the owner's call). describe("PlexDiscover · keyboard", () => { it("registers every rail and the genre grid, and j walks on from rail to rail", async () => { plexDiscover.mockResolvedValue(fullPayload); diff --git a/frontend/src/components/ShareDialog.tsx b/frontend/src/components/ShareDialog.tsx index d9a9f76b..77453bdd 100644 --- a/frontend/src/components/ShareDialog.tsx +++ b/frontend/src/components/ShareDialog.tsx @@ -235,7 +235,7 @@ function LinkShare({ job }: { job: DownloadJob }) { queryFn: () => api.downloadLinks(job.id), }); const [creating, setCreating] = useState(false); - // D1: downloadable by default — matches the one-click Share (which relies on the backend + // Downloadable by default — matches the one-click Share (which relies on the backend // default) and the common "let them have the file" intent. The owner can uncheck it here. const [allowDownload, setAllowDownload] = useState(true); const [expiryDays, setExpiryDays] = useState(null); @@ -260,7 +260,7 @@ function LinkShare({ job }: { job: DownloadJob }) { }); // Open with a link already generated: sharing is almost always the goal, so mint a default - // (no password, no expiry, downloadable per D1) link the moment the dialog finds none — as if the + // (no password, no expiry, downloadable) link the moment the dialog finds none — as if the // user had pressed "New link" → "Create". All further controls (password, download toggle, expiry, // extra links, revoke) stay available on the created link and via the "New link" button. const autoTried = useRef(false); diff --git a/frontend/src/components/UpNextCard.tsx b/frontend/src/components/UpNextCard.tsx index 8b541533..72d15ece 100644 --- a/frontend/src/components/UpNextCard.tsx +++ b/frontend/src/components/UpNextCard.tsx @@ -10,7 +10,7 @@ import { Button } from "./ui/Button"; import { IconButton } from "./ui/IconButton"; // Up-next / end-card. Shown at episode end when a next item exists and the pref is on, INSTEAD -// of the old instant jump — so binge stays under the viewer's control (D6-A). Counts down ~10s then +// of the old instant jump — so binge stays under the viewer's control. Counts down ~10s then // advances; "Play now" advances immediately; Cancel/✕ dismisses and stays on the finished frame. It only // lives on the play-to-the-end path; credits-auto-skip advances earlier and never reaches this card. const COUNTDOWN_S = 10; diff --git a/frontend/src/components/plexCards.dom.test.tsx b/frontend/src/components/plexCards.dom.test.tsx index af05785c..6f508f8c 100644 --- a/frontend/src/components/plexCards.dom.test.tsx +++ b/frontend/src/components/plexCards.dom.test.tsx @@ -258,7 +258,7 @@ describe("EpisodeCard a11y", () => { }); // In a keyboard-walkable grid or rail the WHOLE card is the item (the ring goes round the -// poster and its title, D2) and the poster — the card's own role=button — is what Enter opens. +// poster and its title) and the poster — the card's own role=button — is what Enter opens. describe("Plex cards in a keyboard-walkable list", () => { const cases = [ [ diff --git a/frontend/src/lib/keymap.sequence.test.ts b/frontend/src/lib/keymap.sequence.test.ts index cf32f0d5..7d43cb1c 100644 --- a/frontend/src/lib/keymap.sequence.test.ts +++ b/frontend/src/lib/keymap.sequence.test.ts @@ -87,7 +87,7 @@ describe("two-step sequences", () => { expect(calls).toEqual(["goDownloads"]); }); - // No second step needs a modifier (D1), so a chord with one is the reader changing their mind: it + // No second step needs a modifier (see the page-level rows below), so a chord with one is the reader changing their mind: it // must reach its own row. Swallowed, `g` then Ctrl+K opened the browser's search, not the palette. it("lets a modifier chord abandon the sequence and match as itself", () => { const calls: string[] = []; @@ -148,7 +148,7 @@ describe("page-level scopes under a layer", () => { }); describe("the page-level rows", () => { - // Owner rule D1, enforced rather than commented: on the Hungarian layout the owner types on, a + // The owner's rule, enforced rather than commented: on the Hungarian layout the owner types on, a // chord with a modifier (or `/`, `?`, which ARE Shift chords there) is not a quick key. it("need no modifier and no Shift-only character", () => { for (const s of SHORTCUTS) { diff --git a/frontend/src/lib/planCodes.contract.test.ts b/frontend/src/lib/planCodes.contract.test.ts index 4e3cf420..aefb05ed 100644 --- a/frontend/src/lib/planCodes.contract.test.ts +++ b/frontend/src/lib/planCodes.contract.test.ts @@ -24,10 +24,10 @@ const ALLOWED = new Set([ // Anything else that looks like a code is one — `"""E30:`, `it("E36:`, `# S6:` and `S3's` included. const NOT_CODES = /\bS\d{1,2} ?·? ?E\d+|\btitle[=:] ?"[ES]\d{1,2}"|\bE4\/E7\/E9\b/g; const PLAN_CODES = [ - // an epic (`E28`, `E9.5`, `E28/E30`), an older roadmap item (`R14`), a sprint (`S6`, `S3e2`) or a - // phase (`P1`) — not `E2E`, not a ruff rule (`S110`), not `S3h`, the media-card decision - // docs/DESIGN.md is headed by - /\b(?:E\d{1,2}(?:\.\d+)?|R\d{1,2}|S(?!3h\b)\d{1,2}(?:[a-z]\d?)?|P\d)\b/, + // an epic (`E28`, `E9.5`, `E28/E30`), an older roadmap item (`R14`), a sprint (`S6`, `S3e2`), a + // phase (`P1`) or a plan's decision (`D1`, `D6-A`) — not `E2E`, not a ruff rule (`S110`), not + // `S3h`, the media-card decision docs/DESIGN.md is headed by + /\b(?:E\d{1,2}(?:\.\d+)?|R\d{1,2}|S(?!3h\b)\d{1,2}(?:[a-z]\d?)?|P\d|D\d{1,2})\b/, // a review round (`round 4`, `deep-review`) /\b[Rr]ound[ -]\d|deep-review/, ]; @@ -58,6 +58,9 @@ describe("plan codes", () => { '"""E30: the new sink.', 'it("E36: keeps x", () => {', "// both sides (E28/E30)", + "# D1: a newly shared download is downloadable by default", + "// binge stays under the viewer's control (D6-A)", + "// (owner decision D4)", "# the roadmap's R6/R7 pair", "// held since E9.5", "// (S6) moved it", @@ -79,6 +82,8 @@ describe("plan codes", () => { 'expect(btn.textContent).toContain("S2 · E1");', "// the S3h media card", '"S110", "S112",', + "# the JPEG SOI marker (0xFF 0xD8 0xFF)", + "// a 3D view", ]; expect(cited.filter((l) => !isPlanCode(l))).toEqual([]); expect(clean.filter(isPlanCode)).toEqual([]); diff --git a/scripts/audit_metrics.py b/scripts/audit_metrics.py index 5e86fe3b..7e250918 100644 --- a/scripts/audit_metrics.py +++ b/scripts/audit_metrics.py @@ -27,12 +27,13 @@ BIG_FILE = 800 BE_COMPLEXITY = 10 # ruff's C901 default FE_COMPLEXITY = 20 # the frontend's line -# planCodes.contract.test.ts's definition, copied: an epic, roadmap item, sprint or phase code, or a -# review round; NOT_CODES are its named exceptions, cut first (TV episode notation, a fixture's -# season or episode title, ruff's default `E4/E7/E9`). ASCII, because JavaScript's `\b` is. +# planCodes.contract.test.ts's definition, copied: an epic, roadmap item, sprint, phase or decision +# code, or a review round; NOT_CODES are its named exceptions, cut first (TV episode notation, a +# fixture's season or episode title, ruff's default `E4/E7/E9`). ASCII, because JavaScript's `\b` is. NOT_CODES = re.compile(r'\bS\d{1,2} ?·? ?E\d+|\btitle[=:] ?"[ES]\d{1,2}"|\bE4/E7/E9\b', re.ASCII) PLAN_CODE = re.compile( - r"\b(?:E\d{1,2}(?:\.\d+)?|R\d{1,2}|S(?!3h\b)\d{1,2}(?:[a-z]\d?)?|P\d)\b|\b[Rr]ound[ -]\d|deep-review", + r"\b(?:E\d{1,2}(?:\.\d+)?|R\d{1,2}|S(?!3h\b)\d{1,2}(?:[a-z]\d?)?|P\d|D\d{1,2})\b" + r"|\b[Rr]ound[ -]\d|deep-review", re.ASCII, ) NARRATION = re.compile(r"\b(used to|no longer)\b", re.IGNORECASE) From 5561d9a3792013e333c0243add9016472120f0f6 Mon Sep 17 00:00:00 2001 From: npeter83 Date: Mon, 5 Oct 2026 03:16:27 +0200 Subject: [PATCH 02/12] =?UTF-8?q?feat(downloads):=20every=20ready=20file?= =?UTF-8?q?=20is=20phone-safe=20=E2=80=94=20uploads=20and=20clips=20too?= MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit A share link is opened on whatever the friend holds. Measured on an iPhone and an iPad: AV1, Matroska, HEVC tagged hev1 and MP3 inside an .mp4 fail there; Chrome and Firefox add HEVC and 10-bit / 4:4:4 H.264. The old check judged the codec NAME only and ran on downloads only, so a 10-bit H.264 passed, an upload went out exactly as it came (an iPhone's HEVC .mov, a webm, an mkv) and a fast trim kept its source's codecs. One rule now, `downloads/playable.py`: video is H.264 8-bit 4:2:0 + AAC in .mp4, audio-only is .mp3 or AAC .m4a; `plan` says what must change, `build_cmd` re-encodes only the failing stream (or remuxes). The worker applies it to a download, an editor clip and — new — an upload: media that fails the rule is queued (asset pending, job queued) and the worker converts it, keeping only the converted file (owner's choice). Until then the original sits at rel_path, so Retry starts from it and a delete removes it; an upload that lost its last holder mid-conversion is reaped by the GC. Every libx264 run (edit, concat, conversion) now pins `-pix_fmt yuv420p` — without it a 10-bit source came out High 10. The download cache signature moves to v5 so a cached 10-bit asset re-derives. The upload toast says when the file is still converting (HU/EN). Tests: the plan for every measured class; real ffmpeg turning a 10-bit Matroska + Opus and an mp3-in-mp4 into files the rule accepts (red without the pixel-format pin); the queued upload's delete, cancel-then-GC, failed conversion and real worker conversion (the cleanup tests red without their rules); the toast (red without the branch). Live on localdev: 9 fixture uploads converted, every served file probed H.264 High yuv420p + AAC, or mp3 / m4a. --- backend/app/downloads/edit.py | 117 ++---------- backend/app/downloads/formats.py | 19 +- backend/app/downloads/gc.py | 23 +++ backend/app/downloads/pipeline.py | 171 +++++++++++++----- backend/app/downloads/playable.py | 99 ++++++++++ backend/app/downloads/probe.py | 63 +++++-- backend/app/downloads/runner.py | 4 +- backend/app/downloads/service.py | 12 +- backend/app/downloads/upload.py | 36 ++-- backend/app/routes/downloads.py | 3 +- backend/tests/test_download_deprioritize.py | 14 +- backend/tests/test_download_output.py | 45 +---- backend/tests/test_formats.py | 17 -- backend/tests/test_playable.py | 157 ++++++++++++++++ backend/tests/test_public_unlock_limit.py | 5 +- backend/tests/test_upload.py | 157 +++++++++++++++- .../src/components/UploadDialog.dom.test.tsx | 18 +- frontend/src/components/UploadDialog.tsx | 13 +- frontend/src/i18n/locales/en/downloads.json | 1 + frontend/src/i18n/locales/hu/downloads.json | 1 + frontend/src/lib/api/downloads.ts | 3 +- 21 files changed, 712 insertions(+), 266 deletions(-) create mode 100644 backend/app/downloads/playable.py create mode 100644 backend/tests/test_playable.py diff --git a/backend/app/downloads/edit.py b/backend/app/downloads/edit.py index d191d14a..38c90f5d 100644 --- a/backend/app/downloads/edit.py +++ b/backend/app/downloads/edit.py @@ -188,6 +188,13 @@ def clip_duration(spec: dict, source_duration: int | None) -> int | None: # --- ffmpeg: trim / crop -------------------------------------------------------------------- +# Every libx264 / aac re-encode in this package uses these, so a re-encoded file is always the +# phone-safe shape (`playable`): `-pix_fmt yuv420p` pins 8-bit 4:2:0 — without it libx264 keeps a +# 10-bit or 4:4:4 source's pixel format and writes H.264 High 10 / High 4:4:4, which browsers decode +# only patchily (the one safe H.264 is 8-bit 4:2:0). +X264_ARGS = ["-c:v", "libx264", "-preset", "veryfast", "-crf", "20", "-pix_fmt", "yuv420p"] +AAC_ARGS = ["-c:a", "aac", "-b:a", "192k"] + def build_edit_cmd(src: Path, dest: Path, spec: dict, out_ext: str) -> list[str]: """ffmpeg command for a trim/crop edit. `-progress pipe:1` streams machine-readable progress. @@ -225,18 +232,7 @@ def build_edit_cmd(src: Path, dest: Path, spec: dict, out_ext: str) -> list[str] # the exact cut). A crop adds the filter; an accurate trim re-encodes with no filter. if crop: cmd += ["-vf", f"crop={crop['w']}:{crop['h']}:{crop['x']}:{crop['y']}"] - cmd += [ - "-c:v", - "libx264", - "-preset", - "veryfast", - "-crf", - "20", - "-c:a", - "aac", - "-b:a", - "192k", - ] + cmd += [*X264_ARGS, *AAC_ARGS] else: cmd += ["-c", "copy", "-avoid_negative_ts", "make_zero"] @@ -297,16 +293,8 @@ def build_concat_plan(src: Path, dest: Path, spec: dict, out_ext: str, staging: "[v]", "-map", "[a]", - "-c:v", - "libx264", - "-preset", - "veryfast", - "-crf", - "20", - "-c:a", - "aac", - "-b:a", - "192k", + *X264_ARGS, + *AAC_ARGS, ] else: listp = staging / "concat.txt" @@ -474,14 +462,7 @@ def run_ffmpeg( raise StatedFailure(f"ffmpeg failed ({ret}): {''.join(err_tail)[-300:]}") -# --- ffmpeg: universal browser playability -------------------------------------------------- -# A downloaded file must play in EVERY browser, whatever the source shipped. Universal = H.264 -# video + AAC/MP3 audio in an mp4 container: H.264 is the only video codec all browsers decode -# (VP9/AV1 fail on Safari/iOS; HEVC fails on Chrome/Firefox), and mp4/AAC is the safe pairing. -# Anything else is fixed post-download: re-encode the offending stream(s), or just remux when the -# codecs are already fine but the container isn't. -_BROWSER_SAFE_VCODEC = ("h264",) -_BROWSER_SAFE_ACODEC = ("aac", "mp3") +# --- ffprobe --------------------------------------------------------------------------------- def run_ffprobe_json( @@ -493,8 +474,8 @@ def run_ffprobe_json( ) -> dict | None: """One ffprobe call → its parsed JSON dict, or None on any failure (timeout, non-zero exit, missing binary, unparseable output). The SINGLE ffprobe-JSON-invocation point in the downloads - package: the browser-playability codec probe here and the shared media probe (`probe.probe_media`, - used by both the download worker's finalize and the upload ingest) both go through it, so the + package: the shared media probe (`probe.probe_json`: the stored facts and the phone-safe rule) goes + through it, so the subprocess flags/timeout/error handling live in one place instead of drifting copies.""" args = ["ffprobe", "-v", "error"] if show_streams: @@ -517,35 +498,6 @@ def run_ffprobe_json( return None -def _codecs_from_streams(streams: list) -> tuple[str | None, str | None]: - """(video_codec, audio_codec) lowercased from an ffprobe JSON stream list — the first of each kind. - - Parsed from the JSON `streams` list, NOT ffprobe's flat `default=nw=1` output: there the fields - arrive in ffprobe's own struct order, which puts `codec_name` BEFORE `codec_type`, so a line-by-line - "remember the last codec_type" parser attributes every name to the PREVIOUS stream. Measured on real - downloads: an ordinary H.264+AAC mp4 probed as `v=aac a=None` — the browser-playability check then - believed the video stream was AAC and re-encoded a perfectly fine file on every download. Reading each - stream dict by its own `codec_type` is immune to that ordering.""" - vcodec = acodec = None - for s in streams: - name = (s.get("codec_name") or "").lower() or None - ctype = (s.get("codec_type") or "").lower() - if ctype == "video" and vcodec is None: - vcodec = name - elif ctype == "audio" and acodec is None: - acodec = name - return vcodec, acodec - - -def probe_codecs(path: Path) -> tuple[str | None, str | None]: - """Return (video_codec, audio_codec) lowercased via one ffprobe call; None for a missing - stream or if ffprobe fails (a failed probe is treated as 'leave it alone' upstream).""" - data = run_ffprobe_json(path, entries="stream=codec_type,codec_name") - if data is None: - return None, None - return _codecs_from_streams(data.get("streams") or []) - - def probe_duration(path: Path) -> int: """Media duration in whole seconds via ffprobe, or 0 if unknown — a progress denominator for a re-encode when the yt-dlp info dict carried no `duration` (some URL/live extractions).""" @@ -577,49 +529,6 @@ def probe_duration(path: Path) -> int: return 0 -def browser_transcode_flags( - vcodec: str | None, acodec: str | None, is_mp4: bool -) -> tuple[bool, bool, bool]: - """Decide what (if anything) makes a file universally browser-playable. Returns - (needs_change, reencode_video, reencode_audio). needs_change=False => already H.264 + AAC/MP3 - in mp4. A None codec (no such stream, or a failed probe) is never re-encoded.""" - rv = bool(vcodec) and vcodec not in _BROWSER_SAFE_VCODEC - ra = bool(acodec) and acodec not in _BROWSER_SAFE_ACODEC - return (rv or ra or not is_mp4), rv, ra - - -def build_browser_safe_cmd( - src: Path, dest: Path, reencode_video: bool, reencode_audio: bool, threads: int | None = None -) -> list[str]: - """ffmpeg command that lands `src` as a browser-safe mp4 (faststart): re-encode only the - stream(s) that need it (libx264 / aac), stream-copy the rest, always into mp4. `-progress - pipe:1` streams machine-readable progress for run_ffmpeg. `threads` caps the encoder so a - re-encode can't claim every core on the shared box (see the worker-child de-prioritization).""" - cmd = [ - "ffmpeg", - "-y", - "-hide_banner", - "-loglevel", - "error", - "-nostdin", - "-progress", - "pipe:1", - "-nostats", - "-i", - str(src), - ] - cmd += ( - ["-c:v", "libx264", "-preset", "veryfast", "-crf", "20"] - if reencode_video - else ["-c:v", "copy"] - ) - cmd += ["-c:a", "aac", "-b:a", "192k"] if reencode_audio else ["-c:a", "copy"] - if threads and threads > 0: - cmd += ["-threads", str(threads)] - cmd += ["-movflags", "+faststart", str(dest)] - return cmd - - # --- filmstrip (editor scrub track) --------------------------------------------------------- _SB_COLS = 12 diff --git a/backend/app/downloads/formats.py b/backend/app/downloads/formats.py index a8deef1e..edf5dc8a 100644 --- a/backend/app/downloads/formats.py +++ b/backend/app/downloads/formats.py @@ -182,7 +182,9 @@ _DEFAULTS = { # v4: EVERY download is clamped to the mp4/H.264/AAC compat profile (no more mkv/webm/VP9/AV1/Opus # opt-out), so any asset cached under an old non-compat spec — or a stale non-H.264 mp4 that slipped # the v3 net — must re-derive through the current browser-safe guarantee. -_SIG_VERSION = 4 +# v5: the phone-safe rule (`playable`) also judges the pixel format, so a cached 10-bit / 4:4:4 H.264 +# asset (which v4 let through as "H.264") re-derives as 8-bit 4:2:0 instead of being reused. +_SIG_VERSION = 5 # Audio-only downloads version on their own, so a change to the audio bytes doesn't re-download every # cached video. v5: an extracted .mp3/.m4a is stored as itself instead of remuxed into an .mp4 that # iPad's WebKit refuses to play, so a cached remuxed audio asset must re-derive. @@ -219,12 +221,13 @@ def normalize(spec: dict) -> dict: s["embed_subs"] = False # Browser-playability guarantee: the Download module exists to download → share → PLAY # in-browser, so every output must play in EVERY browser, iOS/Safari/WebKit included. The only - # universally-safe combo is mp4 + H.264 + AAC/MP3, so we CLAMP the spec to it HERE — the single - # point every path (picker, stored profile, raw API) flows through — instead of only hiding - # options in the UI. Non-mp4 containers (mkv doesn't play at all; webm/VP9/AV1 fail iOS) and any - # explicit vcodec are coerced to the compat profile; Opus audio (Safari-flaky as a bare file) - # falls back to AAC/m4a. A source that exposes no H.264 is still caught by the worker's - # browser-safe re-encode (`_ensure_browser_playable`), so playback is guaranteed end-to-end. + # universally-safe combo is mp4 + H.264 + AAC (an .mp3/.m4a for audio), so we CLAMP the spec to + # it HERE — the single point every path (picker, stored profile, raw API) flows through — instead + # of only hiding options in the UI. Non-mp4 containers (mkv plays nowhere on iOS; AV1 fails there, + # and webm/VP9 plays only on recent devices) and any explicit vcodec are coerced to the compat + # profile; Opus audio falls back to AAC/m4a. A source that exposes no H.264 (or a 10-bit one) is + # still caught by the worker's phone-safe step (`downloads.playable`), so playback is guaranteed + # end-to-end. s["container"] = "mp4" s["vcodec"] = None if s["audio_format"] not in ("m4a", "mp3"): @@ -351,7 +354,7 @@ def build_ydl_opts( # inside mp4 — a VP9/AV1-in-mp4 file shows a broken player there — so H.264 is selected at the # SELECTION level, not merely preferred: a sort key can't conjure an H.264 stream a source # doesn't expose as video-only. A source that ships only non-H.264 is caught after download by - # the worker's browser-safe re-encode (`_ensure_browser_playable`). + # the worker's phone-safe step (`downloads.playable`). if s["mode"] == "v": # Video-only: keep it audio-free. Prefer an H.264 video-only stream, then any video-only, # and only fall to a progressive (audio-bearing) `best` as a last resort. diff --git a/backend/app/downloads/gc.py b/backend/app/downloads/gc.py index 60267f98..8218cf40 100644 --- a/backend/app/downloads/gc.py +++ b/backend/app/downloads/gc.py @@ -232,6 +232,29 @@ def run_download_gc(db: Session) -> dict: if orphans: db.commit() + # 4b. Reap unheld uploads. An upload carries no expires_at and is never re-enqueued, so the passes + # above never free one that lost its last holder outside `release_asset` — a conversion that + # finished after its job was cancelled, or one cancelled mid-run (the asset was `downloading` + # then). Its file, ready or still the unconverted original, sits at `rel_path`. + unheld = ( + db.execute( + select(MediaAsset).where( + MediaAsset.source_kind == "upload", + MediaAsset.ref_count == 0, + MediaAsset.status != "downloading", + ) + ) + .scalars() + .all() + ) + for asset in unheld: + if asset.rel_path: # no holder is left to tell, so no notice — just the file and the row + storage.delete_asset_files(settings.download_root, asset.rel_path) + db.delete(asset) + reaped += 1 + if unheld: + db.commit() + # 5. Orphan FILE sweep — remove on-disk cruft no surviving asset owns (old accented dirs, stale # posters). Runs last so the keep-set reflects the assets that survived passes 2-4. swept = _sweep_orphan_files(db, settings.download_root) diff --git a/backend/app/downloads/pipeline.py b/backend/app/downloads/pipeline.py index e2554fbe..3d3510ea 100644 --- a/backend/app/downloads/pipeline.py +++ b/backend/app/downloads/pipeline.py @@ -13,7 +13,8 @@ from app.config import settings from app.db import SessionLocal from app.downloads import edit as editmod from app.downloads import errors as dlerrors -from app.downloads import adapters, credentials, formats, mediakind, probe, quota, service, storage +from app.downloads import adapters, credentials, formats, mediakind, playable, probe, quota +from app.downloads import service, storage from app.downloads import urlguard from app.downloads.jobdb import ( _edit_staging_dir, @@ -416,48 +417,50 @@ def _download_settings() -> tuple[str, int]: ) -def _ensure_browser_playable(job_id: int, produced: Path, spec: dict, info: dict) -> Path: - """Make an mp4-compat-profile download play in every browser: H.264 + AAC/MP3 in mp4. If the - download (whatever the source shipped) isn't already that, re-encode the offending stream(s) — - or just remux when the codecs are fine but the container isn't. **Scope:** only the mp4 compat - profile is normalized; an explicit `vcodec` or a non-mp4 custom profile is the user opting out - of the guarantee and is left as-is (so a `webm` profile can still ship VP9/Opus). Returns the - file to store (converted, or the original) and corrects `info`'s codec fields to match disk. +def _ensure_phone_safe( + job_id: int, + produced: Path, + out_dir: Path, + phase: str, + total_s: int | None = None, + *, + keep_source: bool = False, +) -> Path: + """Bring a file that is about to become a ready library asset into the phone-safe shape + (`playable`), writing any converted copy into `out_dir`. Returns the file to store — `produced` + itself when it already is that shape, or isn't media at all — and removes `produced` once a + converted copy exists, unless `keep_source` (an upload's original, which must outlive the + conversion until the database points at the new file). The job shows `phase` with real ffmpeg + progress while it converts. - An audio download (mode "a") is never touched: yt-dlp already extracted it to .mp3 / .m4a, which - every browser plays as audio/mpeg / audio/mp4. Remuxing it into .mp4 served an mp3 as video/mp4, - which iPad's WebKit refuses to play (prod job 268).""" - if spec.get("mode") == "a" or spec.get("container") != "mp4" or spec.get("vcodec"): + Every ready path calls this — a download, an upload's worker conversion and an editor clip — so + a share link can only ever serve the one shape every phone and browser plays.""" + p = playable.plan(probe.probe_json(produced), produced.suffix) + if p is None or not p.needs_change: return produced - vcodec, acodec = editmod.probe_codecs(produced) - is_mp4 = produced.suffix.lower() == ".mp4" - needs, reenc_v, reenc_a = editmod.browser_transcode_flags(vcodec, acodec, is_mp4) - if not needs: - return produced - - dest = produced.with_name(produced.stem + ".browser.mp4") - action = "re-encode" if (reenc_v or reenc_a) else "remux" + dest = out_dir / f"{produced.stem}.safe.{p.ext}" + action = "re-encode" if (p.reencode_video or p.reencode_audio) else "remux" log.info( - "job %s: %s (v=%s a=%s mp4=%s) -> browser-safe mp4", job_id, action, vcodec, acodec, is_mp4 + "job %s: %s (video=%s audio=%s) -> phone-safe .%s", + job_id, + action, + p.reencode_video, + p.reencode_audio, + p.ext, + extra=logs.event("download.phone_safe", job_id=job_id, action=action, ext=p.ext), ) - _set_job(job_id, phase="processing", progress=0, speed_bps=None, eta_s=None) - # Prefer the info duration; probe the file when the extractor didn't provide one, so a long - # re-encode still shows real progress instead of a frozen 0%. - total_s = int(info["duration"]) if info.get("duration") else editmod.probe_duration(produced) + _set_job(job_id, phase=phase, progress=0, speed_bps=None, eta_s=None) + # A re-encode with no known length would hold a frozen 0%; probe the file for a denominator. editmod.run_ffmpeg( - editmod.build_browser_safe_cmd( - produced, dest, reenc_v, reenc_a, threads=editmod.encode_thread_cap() - ), - total_s=total_s, - on_progress=_ffmpeg_progress_writer(job_id, "processing"), + playable.build_cmd(produced, dest, p, threads=editmod.encode_thread_cap()), + total_s=total_s or editmod.probe_duration(produced), + on_progress=_ffmpeg_progress_writer(job_id, phase), should_cancel=_cancel_poll(job_id), ) - produced.unlink(missing_ok=True) - # Keep the stored asset metadata truthful about what's now on disk. - if reenc_v: - info["vcodec"] = "h264" - if reenc_a: - info["acodec"] = "aac" + if not dest.exists(): + raise StatedFailure("ffmpeg produced no output file") + if not keep_source: + produced.unlink(missing_ok=True) return dest @@ -522,7 +525,13 @@ def _download( adapter = adapters.resolve_adapter(source_kind, source_ref, info) produced = _produced_file(info, staging) - produced = _ensure_browser_playable(job_id, produced, spec, info) + produced = _ensure_phone_safe( + job_id, + produced, + staging, + "processing", + int(info["duration"]) if info.get("duration") else None, + ) ext = produced.suffix.lstrip(".").lower() # Codecs/dims/duration come from a PROBE of the produced file — the ground truth — NOT yt-dlp's # `info` dict, whose fields are absent for some sources (Facebook among them): deriving @@ -722,9 +731,12 @@ def _log_download_failed(job_id: int, msg: str, code: str | None, exc: BaseExcep ) -def _fail_edit(job_id: int, asset_id: int, msg: str, exc: BaseException | None = None) -> None: - """Fail an edit job, and say so ONCE — every way an edit fails comes through here, so this is - where its line and its event code live. `exc` is the exception when there was one. +def _fail_edit( + job_id: int, asset_id: int, msg: str, exc: BaseException | None = None, *, what: str = "edit" +) -> None: + """Fail a local ffmpeg job — an edit, or an upload's conversion (`what="upload"`) — and say so + ONCE: every way one fails comes through here, so this is where its line and its event code + (`.job.failed`) live. `exc` is the exception when there was one. Its traceback goes with the line unless the message already tells it all (`_failure_stack`). Logged after the write, as on the download error path.""" @@ -746,12 +758,13 @@ def _fail_edit(job_id: int, asset_id: int, msg: str, exc: BaseException | None = # `msg` is the user's copy; ffmpeg's stderr tail in it can name the source file, whose path is # built from the channel and the title. log.warning( - "edit job %s failed: %s", + "%s job %s failed: %s", + what, job_id, formats.log_safe(msg), extra={ **logs.event( - "edit.job.failed", + f"{what}.job.failed", job_id=job_id, asset_id=asset_id, **({"error_type": type(exc).__name__} if exc is not None else {}), @@ -811,6 +824,10 @@ def _process_edit(job_id: int, asset_id: int) -> None: ) if not dest_staging.exists(): raise StatedFailure("ffmpeg produced no output file") + # A fast trim stream-copies the source's codecs and container, so the clip is only as + # playable as its source: hold it to the same rule as every other ready file. + dest_staging = _ensure_phone_safe(job_id, dest_staging, staging, "editing", total_s) + out_ext = dest_staging.suffix.lstrip(".").lower() or out_ext rel = storage.edit_rel_path(user_id, asset_id, out_ext) storage.place_file(dest_staging, settings.download_root, rel) @@ -867,3 +884,73 @@ def _process_edit(job_id: int, asset_id: int) -> None: _fail_edit(job_id, asset_id, str(exc)[:500], exc) finally: shutil.rmtree(staging, ignore_errors=True) + + +def _convert_upload(job_id: int, asset_id: int) -> None: + """Convert a queued upload into the phone-safe shape (`playable`) and make it ready. + + The original sits at the asset's `rel_path` (see `upload`); the converted file replaces it there + under the new extension, and only then is the original removed — so a failed or stopped + conversion leaves the original in place for Retry, and a delete still finds it by `rel_path`.""" + with SessionLocal() as db: + asset = db.get(MediaAsset, asset_id) + old_rel = asset.rel_path if asset else None + duration = asset.duration_s if asset else None + src = storage.safe_abs_path(settings.download_root, old_rel) if old_rel else None + if old_rel is None or src is None or not src.exists(): + _fail_edit(job_id, asset_id, "The uploaded file is missing.", what="upload") + return + staging = _staging_dir(asset_id) + try: + shutil.rmtree(staging, ignore_errors=True) + staging.mkdir(parents=True, exist_ok=True) + safe = _ensure_phone_safe(job_id, src, staging, "processing", duration, keep_source=True) + ext = safe.suffix.lstrip(".").lower() + new_rel = Path(old_rel).with_suffix(f".{ext}").as_posix() + if safe != src: + storage.place_file(safe, settings.download_root, new_rel) + final = storage.abs_path(settings.download_root, new_rel) + vcodec, acodec, width, height, dur = probe.probe_media(final) + with SessionLocal() as db: + asset = db.get(MediaAsset, asset_id) + if asset: + asset.status = "ready" + asset.rel_path = new_rel + asset.container = ext + asset.size_bytes = final.stat().st_size + asset.vcodec, asset.acodec = vcodec, acodec + asset.width, asset.height = width or asset.width, height or asset.height + asset.duration_s = dur or asset.duration_s + asset.error = None + asset.last_access_at = datetime.now(timezone.utc) + job = db.get(DownloadJob, job_id) + owner_id = job.user_id if job else None + title = asset.title if asset else None + finalized = service.finalize_if_running( + db, job_id, status=JobStatus.DONE, progress=100, phase=None + ) + db.commit() + if finalized: + _notify_download_done(db, owner_id, title, job_id, kind="download") + if new_rel != old_rel: + src.unlink(missing_ok=True) + if finalized: + log.info( + "upload job %s converted: asset %s", + job_id, + asset_id, + extra=logs.event("upload.job.done", job_id=job_id, asset_id=asset_id, ext=ext), + ) + except editmod.EditAborted: + # Paused/canceled mid-encode: the original is untouched; free the asset for a later retry. + _reset_asset_pending(asset_id) + log.info( + "upload job %s conversion aborted (pause/cancel)", + job_id, + extra=logs.event("upload.job.aborted", job_id=job_id), + ) + except Exception as exc: # noqa: BLE001 — surface the failure, keep the worker alive + # silent-ok: not silent — `_fail_edit` is the report, and it takes `exc` for its stack. + _fail_edit(job_id, asset_id, str(exc)[:500], exc, what="upload") + finally: + shutil.rmtree(staging, ignore_errors=True) diff --git a/backend/app/downloads/playable.py b/backend/app/downloads/playable.py new file mode 100644 index 00000000..8de66bcb --- /dev/null +++ b/backend/app/downloads/playable.py @@ -0,0 +1,99 @@ +"""The phone-safe rule: what a library file must be so it plays wherever a share link lands. + +A shared `/watch` link is opened by whoever got it, on whatever they hold: an iPhone's WebKit, an +Android Chrome, a desktop Firefox. The one shape every one of them plays: + + * video → H.264, 8-bit 4:2:0 (`yuv420p`, or its full-range twin `yuvj420p`), AAC audio, in .mp4 + * audio only → MP3 in .mp3, or AAC in .m4a + +Measured on the owner's iPhone + iPad (2026-10-05): AV1, Matroska, HEVC tagged `hev1` and MP3 +inside an .mp4 all fail there. VP9/WebM, HEVC `hvc1` and 10-bit H.264 played on those devices, but +Chrome and Firefox do not decode HEVC and browsers decode 10-bit / 4:4:4 H.264 only patchily, so the +rule is the shape every browser plays, not the shape one device plays. + +Every file that becomes a ready library asset passes through here — a download, an upload (the +worker converts it, see `upload`), and an editor clip — so a link can only ever serve this shape. +`plan` says what (if anything) must change; `build_cmd` is the one ffmpeg command that changes it: +re-encode only the stream that fails, copy the rest, or just remux when only the container is wrong. +""" + +from dataclasses import dataclass +from pathlib import Path + +from app.downloads import edit as editmod +from app.downloads import probe + +_SAFE_VCODEC = "h264" +_SAFE_PIX_FMTS = frozenset({"yuv420p", "yuvj420p"}) +_VIDEO_EXT = "mp4" +# An audio-only file: the codec, and the extension it plays in. Anything else becomes AAC in .m4a. +_AUDIO_HOME = {"mp3": "mp3", "aac": "m4a"} +_AUDIO_FALLBACK_EXT = "m4a" + + +@dataclass(frozen=True) +class Plan: + """What makes one file phone-safe. `ext` is the extension the safe file has (no dot).""" + + ext: str + has_video: bool + reencode_video: bool + reencode_audio: bool + needs_change: bool + + +def plan(data: dict | None, ext: str) -> Plan | None: + """The phone-safe plan for a file whose `probe.probe_json` is `data` and whose extension is + `ext`; None when it is not media at all (an unreadable probe, a still image, no audio or video + stream) — such a file is a download-only 'file' and never plays inline, so there is nothing to + make safe.""" + if data is None: + return None + video, audio = probe.primary_streams(data) + ext = ext.lower().lstrip(".") + if video is not None: + rv = (video.get("codec_name") or "").lower() != _SAFE_VCODEC or ( + video.get("pix_fmt") or "" + ).lower() not in _SAFE_PIX_FMTS + ra = audio is not None and (audio.get("codec_name") or "").lower() != "aac" + return Plan(_VIDEO_EXT, True, rv, ra, rv or ra or ext != _VIDEO_EXT) + if audio is not None: + home = _AUDIO_HOME.get((audio.get("codec_name") or "").lower()) + target = home or _AUDIO_FALLBACK_EXT + return Plan(target, False, False, home is None, home is None or ext != target) + return None + + +def build_cmd(src: Path, dest: Path, p: Plan, threads: int | None = None) -> list[str]: + """The ffmpeg command that writes `src` to `dest` (whose suffix is `p.ext`) in the phone-safe + shape: the failing stream(s) re-encoded with the package's shared encoder settings, the rest + stream-copied. An audio-only result drops any cover-art stream. `-progress pipe:1` + streams machine-readable progress; `threads` caps the encoder so a re-encode cannot claim every + core on the shared box (see the worker-child de-prioritization).""" + cmd = [ + "ffmpeg", + "-y", + "-hide_banner", + "-loglevel", + "error", + "-nostdin", + "-progress", + "pipe:1", + "-nostats", + "-i", + str(src), + ] + # Exactly the two streams `plan` judged: `V` is a video stream that is not cover art. Nothing + # else rides along — a subtitle or data track the .mp4 muxer cannot take would fail the run. + if p.has_video: + cmd += ["-map", "0:V:0", "-map", "0:a:0?"] + cmd += editmod.X264_ARGS if p.reencode_video else ["-c:v", "copy"] + else: + cmd += ["-map", "0:a:0"] + cmd += editmod.AAC_ARGS if p.reencode_audio else ["-c:a", "copy"] + if threads and threads > 0: + cmd += ["-threads", str(threads)] + if p.ext in ("mp4", "m4a"): + cmd += ["-movflags", "+faststart"] + cmd += [str(dest)] + return cmd diff --git a/backend/app/downloads/probe.py b/backend/app/downloads/probe.py index 04dd70d3..3e29d157 100644 --- a/backend/app/downloads/probe.py +++ b/backend/app/downloads/probe.py @@ -34,6 +34,28 @@ _IMAGE_FORMATS = { } +def primary_streams(data: dict) -> tuple[dict | None, dict | None]: + """(video stream, audio stream) — the first of each kind — from a NON-None ffprobe JSON dict, by + the same reading the stored facts use: a still image / image-sequence container has neither (it + is a generic 'file'), and a video stream flagged `attached_pic` (an mp3/m4a's cover art) is not a + video track. The phone-safe rule (`playable`) judges exactly these two streams.""" + fmt_names = { + n.strip() for n in ((data.get("format") or {}).get("format_name") or "").split(",") + } + if fmt_names & _IMAGE_FORMATS: + return None, None + video = audio = None + for stream in data.get("streams", []): + kind = stream.get("codec_type") + if kind == "video" and video is None: + if (stream.get("disposition") or {}).get("attached_pic"): + continue # cover art, not a video track + video = stream + elif kind == "audio" and audio is None: + audio = stream + return video, audio + + def _facts_from_probe( data: dict, ) -> tuple[str | None, str | None, int | None, int | None, int | None]: @@ -45,19 +67,11 @@ def _facts_from_probe( fmt_names = {n.strip() for n in (fmt.get("format_name") or "").split(",")} if fmt_names & _IMAGE_FORMATS: return None, None, None, None, None - - vcodec = acodec = None - width = height = None - for stream in data.get("streams", []): - kind = stream.get("codec_type") - if kind == "video" and vcodec is None: - if (stream.get("disposition") or {}).get("attached_pic"): - continue # cover art, not a video track - vcodec = stream.get("codec_name") - width = stream.get("width") - height = stream.get("height") - elif kind == "audio" and acodec is None: - acodec = stream.get("codec_name") + video, audio = primary_streams(data) + vcodec = video.get("codec_name") if video else None + width = video.get("width") if video else None + height = video.get("height") if video else None + acodec = audio.get("codec_name") if audio else None duration = None try: @@ -71,16 +85,29 @@ def _facts_from_probe( return vcodec, acodec, width, height, duration -def probe_media(path: Path) -> tuple[str | None, str | None, int | None, int | None, int | None]: - """(vcodec, acodec, width, height, duration_s) from one ffprobe call; all None if ffprobe fails - or the file isn't playable media (an unprobeable file — or a still image — is treated as a generic - non-media file, so it becomes a download-only 'file' rather than a broken 'video').""" - data = editmod.run_ffprobe_json(path, show_streams=True, show_format=True) +def probe_json(path: Path) -> dict | None: + """The full ffprobe JSON (streams + format) of `path`, or None when ffprobe could not read it — + the one call both the stored facts (`facts`) and the phone-safe rule read from.""" + return editmod.run_ffprobe_json(path, show_streams=True, show_format=True) + + +def facts( + data: dict | None, +) -> tuple[str | None, str | None, int | None, int | None, int | None]: + """(vcodec, acodec, width, height, duration_s) of a `probe_json` result; all None when it is None + (an unprobeable file is treated as a generic non-media file, so it becomes a download-only 'file' + rather than a broken 'video').""" if data is None: return None, None, None, None, None return _facts_from_probe(data) +def probe_media(path: Path) -> tuple[str | None, str | None, int | None, int | None, int | None]: + """(vcodec, acodec, width, height, duration_s) from one ffprobe call; all None if ffprobe fails + or the file isn't playable media (a still image is a generic 'file', see `primary_streams`).""" + return facts(probe_json(path)) + + def resolve_media_facts( path: Path, info: dict ) -> tuple[str | None, str | None, int | None, int | None, int | None]: diff --git a/backend/app/downloads/runner.py b/backend/app/downloads/runner.py index b609e32e..654de3e8 100644 --- a/backend/app/downloads/runner.py +++ b/backend/app/downloads/runner.py @@ -95,12 +95,14 @@ def _run(job_id, asset_id, job_kind, source_kind, source_ref, spec, user_id) -> (`cause=child_exit`).""" # Imported here (not at module top) so the child pays the yt-dlp import cost only when it has # real work, and to keep the import graph obvious: runner is a thin entry over the pipeline. - from app.downloads.pipeline import _download, _process_edit + from app.downloads.pipeline import _convert_upload, _download, _process_edit with formats.hiding(source_ref): try: if job_kind == "edit": _process_edit(job_id, asset_id) + elif job_kind == "upload": + _convert_upload(job_id, asset_id) else: _download(job_id, asset_id, source_kind, source_ref, spec, user_id) except Exception as exc: # noqa: BLE001 — whatever it is, it is reported here or raw diff --git a/backend/app/downloads/service.py b/backend/app/downloads/service.py index f643d04d..22c2a019 100644 --- a/backend/app/downloads/service.py +++ b/backend/app/downloads/service.py @@ -318,14 +318,22 @@ def release_asset(db: Session, job: DownloadJob) -> None: re-enqueuing the same (source,format) — get_or_create_asset would reset that row to `pending` and +1 it, and deleting it under that would FK-null the new job's asset (a lost update on ref_count). A ready asset is still freed at ref==0 (its file is the point); an orphaned errored row is cheap - (no file) and gets reused+reset by the next enqueue, or reclaimed by a later GC pass.""" + (no file) and gets reused+reset by the next enqueue, or reclaimed by a later GC pass. + + An UPLOAD is the exception: it is never shared or re-enqueued (its ref is unique), and while it + waits for or failed its phone-safe conversion the original file already sits at `rel_path` — so + it is freed at ref==0 whatever its state, except mid-conversion (`downloading`: the worker child + still reads it; the GC upload pass takes it once the child has stopped).""" if not job.asset_id or job.status not in HOLDING: return asset = db.get(MediaAsset, job.asset_id) if asset is None: return new_count = decr_ref_count(db, asset.id) - if new_count == 0 and asset.status == "ready": + freeable = asset.status == "ready" or ( + asset.source_kind == "upload" and asset.status != "downloading" + ) + if new_count == 0 and freeable: if asset.rel_path: storage.delete_asset_files(settings.download_root, asset.rel_path) db.delete(asset) diff --git a/backend/app/downloads/upload.py b/backend/app/downloads/upload.py index ea565c50..e7ebd443 100644 --- a/backend/app/downloads/upload.py +++ b/backend/app/downloads/upload.py @@ -2,14 +2,21 @@ A user-uploaded file joins the SAME library as a downloaded one, so it inherits the whole function set already built (metadata chips, share links, and — for media — in-app play/edit). Unlike a -download it bypasses yt-dlp AND the worker entirely: the file is already on the host, so ingest -probes it, places it on disk, and creates an already-`ready` MediaAsset + `done` DownloadJob in one -step — the same "the file exists, mark it done now" shortcut `service.enqueue` takes on a cache hit. +download it bypasses yt-dlp: the file is already on the host, so ingest probes it and places it on +disk. A file that already is the phone-safe shape (`playable`) — or is not media at all — becomes an +already-`ready` MediaAsset + `done` DownloadJob in one step, the same "the file exists, mark it done +now" shortcut `service.enqueue` takes on a cache hit. + +Media that is NOT phone-safe (an iPhone's HEVC .mov, a VP9 .webm, an .mkv) is placed as it came and +queued: the asset stays `pending` and the job `queued`, and the download worker converts it +(`pipeline._convert_upload`) — so a share link never hands a friend a file their phone cannot play. +Only the converted file is kept (the owner's choice: one file per upload; a 10-bit HDR original +becomes 8-bit SDR). Until then the original sits at the asset's `rel_path`, which is what a failed +conversion's Retry starts from again, and what a delete removes (`service.release_asset`). Kinds (the invariant lives in `mediakind`): a probe with a real video stream -> video, audio-only -> audio, neither -> a generic 'file' (pdf/zip/docx/…) that is download-only and never rendered -inline (see the force-download serving in the file/watch routes). This module is the ingest seam a -future sub-function (e.g. convert) plugs a new adapter into, mirroring how it sidesteps the worker. +inline (see the force-download serving in the file/watch routes). An upload does NOT get an `expires_at`: it's user-provided and may be irreplaceable (there's no source to re-download), so it doesn't age out on the retention clock the way a re-fetchable download @@ -24,7 +31,8 @@ from sqlalchemy.orm import Session from app.config import settings from app.downloads import edit as editmod -from app.downloads import mediakind, probe, quota, service, storage +from app.downloads import mediakind, playable, probe, quota, service, storage +from app.downloads.status import JobStatus from app.models import DownloadJob, MediaAsset, display_text @@ -44,15 +52,19 @@ def ingest_upload( size: int, display_name: str | None = None, ) -> DownloadJob: - """Ingest an already-uploaded file at `src_path` into `user_id`'s library and return the `done` - job. `src_path` is MOVED into place on success (the caller stages it to a temp file first). + """Ingest an already-uploaded file at `src_path` into `user_id`'s library and return its job — + `done`, or `queued` for the worker's phone-safe conversion (see the module docstring). `src_path` + is MOVED into place on success (the caller stages it to a temp file first). Raises quota.QuotaExceeded (holdings/footprint cap) — checked BEFORE any disk write, so a rejected upload leaves nothing behind and the caller just cleans up its temp file.""" quota.check_upload(db, user_id, size) # the file's size is known → charge it up front - vcodec, acodec, width, height, duration = probe.probe_media(src_path) + data = probe.probe_json(src_path) + vcodec, acodec, width, height, duration = probe.facts(data) kind = mediakind.derive(vcodec, acodec) + safe = playable.plan(data, Path(filename).suffix) if kind != mediakind.FILE else None + convert = safe is not None and safe.needs_change # An upload never dedups (each file is its own asset): a random ref makes the cache key unique, # so `format_sig` is a constant marker rather than a format hash. @@ -73,7 +85,7 @@ def ingest_upload( final = storage.abs_path(settings.download_root, rel) actual_size = final.stat().st_size if final.exists() else size - asset.status = "ready" + asset.status = "pending" if convert else "ready" asset.rel_path = rel asset.size_bytes = actual_size asset.container = storage.upload_ext(filename) @@ -100,8 +112,8 @@ def ingest_upload( source_ref=ref, profile_snapshot={}, display_name=display_text(display_name), - status="done", - progress=100, + status=JobStatus.QUEUED if convert else JobStatus.DONE, + progress=0 if convert else 100, queue_pos=service.next_queue_pos(db), ) db.add(job) diff --git a/backend/app/routes/downloads.py b/backend/app/routes/downloads.py index d2e1c5d2..49e656dc 100644 --- a/backend/app/routes/downloads.py +++ b/backend/app/routes/downloads.py @@ -502,7 +502,8 @@ def upload_download( (video/audio) gets the full function set, a generic file is download-only. The body IS the file (see the header note above); `spool_upload` has already authenticated the caller and streamed it to a temp path under the download volume, so what is left here is the non-yt-dlp ingest which - probes + places it and returns an already-`done` job. + probes + places it and returns its job — `done`, or `queued` for the worker's phone-safe + conversion (`downloads.upload`). A sync (threadpool) handler on purpose: the ingest does blocking work — an ffprobe of up to 30s plus sync DB writes — so it must NOT run on the event loop. The streaming half is async because diff --git a/backend/tests/test_download_deprioritize.py b/backend/tests/test_download_deprioritize.py index 0264a4c5..1e16a25c 100644 --- a/backend/tests/test_download_deprioritize.py +++ b/backend/tests/test_download_deprioritize.py @@ -8,6 +8,7 @@ import pytest from app.config import settings from app.downloads import edit as editmod +from app.downloads import playable from app.downloads import runner @@ -32,11 +33,12 @@ class TestEncodeThreadCap: assert editmod.encode_thread_cap() == 1 # max(1, 2 - 1) -class TestBrowserSafeThreads: +_REENCODE_VIDEO = playable.Plan("mp4", True, True, False, True) + + +class TestPhoneSafeThreads: def test_threads_flag_added_when_capped(self): - cmd = editmod.build_browser_safe_cmd( - "/in.mkv", "/out.mp4", reencode_video=True, reencode_audio=False, threads=2 - ) + cmd = playable.build_cmd("/in.mkv", "/out.mp4", _REENCODE_VIDEO, threads=2) assert "-threads" in cmd assert cmd[cmd.index("-threads") + 1] == "2" # The cap precedes the output file, so it binds the encoder (not input decoding). @@ -44,9 +46,7 @@ class TestBrowserSafeThreads: @pytest.mark.parametrize("threads", [0, None]) def test_no_threads_flag_when_uncapped(self, threads): - cmd = editmod.build_browser_safe_cmd( - "/in.mkv", "/out.mp4", reencode_video=True, reencode_audio=False, threads=threads - ) + cmd = playable.build_cmd("/in.mkv", "/out.mp4", _REENCODE_VIDEO, threads=threads) assert "-threads" not in cmd diff --git a/backend/tests/test_download_output.py b/backend/tests/test_download_output.py index 9d888b75..4e286817 100644 --- a/backend/tests/test_download_output.py +++ b/backend/tests/test_download_output.py @@ -6,7 +6,7 @@ with "PosixPath('.') has an empty name" instead of downloading.""" import pytest -from app.downloads.edit import _codecs_from_streams, _concat_quote, run_ffprobe_json +from app.downloads.edit import _concat_quote, run_ffprobe_json from app.downloads.pipeline import _media_info, _produced_file from pathlib import Path, PurePosixPath @@ -105,46 +105,9 @@ class TestMediaInfo: assert _media_info(info)["id"] == "vid" -class TestCodecsFromStreams: - # ffprobe emits codec_name BEFORE codec_type, so the old line parser shifted every name onto - # the previous stream and an H.264+AAC mp4 came back as (aac, None) → a needless re-encode. - # _codecs_from_streams reads each stream dict by its own codec_type, immune to that ordering. - def test_reads_each_stream_by_its_own_type(self): - streams = [ - {"codec_name": "h264", "codec_type": "video"}, - {"codec_name": "aac", "codec_type": "audio"}, - ] - assert _codecs_from_streams(streams) == ("h264", "aac") - - def test_audio_first_ordering_is_handled(self): - streams = [ - {"codec_name": "AAC", "codec_type": "audio"}, - {"codec_name": "H264", "codec_type": "video"}, - ] - assert _codecs_from_streams(streams) == ("h264", "aac") - - def test_extra_streams_do_not_win_over_the_first_of_their_kind(self): - streams = [ - {"codec_name": "h264", "codec_type": "video"}, - {"codec_name": "vp9", "codec_type": "video"}, - {"codec_name": "mov_text", "codec_type": "subtitle"}, - {"codec_name": "opus", "codec_type": "audio"}, - ] - assert _codecs_from_streams(streams) == ("h264", "opus") - - def test_audio_only_and_video_only(self): - assert _codecs_from_streams([{"codec_name": "opus", "codec_type": "audio"}]) == ( - None, - "opus", - ) - assert _codecs_from_streams([{"codec_name": "av1", "codec_type": "video"}]) == ("av1", None) - - def test_empty_stream_list(self): - assert _codecs_from_streams([]) == (None, None) - - class TestRunFfprobeJson: - """The single ffprobe-JSON invocation point (shared by probe_codecs + probe.probe_media). A failed + """The single ffprobe-JSON invocation point (`probe.probe_json`, which the stored facts and the + phone-safe rule both read). A failed probe or unparseable output must degrade to None, not raise — the malformed-input safety the old parse_probe_streams string parser used to own now lives here.""" @@ -174,7 +137,7 @@ class TestRunFfprobeJson: monkeypatch.setattr(subprocess, "run", lambda *a, **k: _R()) data = run_ffprobe_json(Path("x"), entries="stream=codec_type,codec_name") - assert _codecs_from_streams(data["streams"]) == ("h264", None) + assert data["streams"] == [{"codec_name": "h264", "codec_type": "video"}] class TestConcatQuote: diff --git a/backend/tests/test_formats.py b/backend/tests/test_formats.py index abf79b1c..ab461d42 100644 --- a/backend/tests/test_formats.py +++ b/backend/tests/test_formats.py @@ -94,23 +94,6 @@ def test_audio_only_unaffected(): assert _opts({"mode": "a", "audio_format": "m4a"})["format"] == "bestaudio/best" -def test_browser_transcode_flags_universal_playability(): - F = editmod.browser_transcode_flags - # Already universal: H.264 + AAC in mp4 → no change. - assert F("h264", "aac", True) == (False, False, False) - assert F("h264", "mp3", True) == (False, False, False) - # Non-H.264 video → full video re-encode (AV1/VP9 fail iOS; HEVC fails Chrome/FF). - assert F("av1", "aac", True) == (True, True, False) - assert F("vp9", "opus", True) == (True, True, True) - assert F("hevc", "aac", True) == (True, True, False) - # H.264 but bad audio → audio-only transcode (video copied). - assert F("h264", "opus", True) == (True, False, True) - # Codecs fine but wrong container → remux only (no re-encode). - assert F("h264", "aac", False) == (True, False, False) - # A failed probe / missing stream is never re-encoded. - assert F(None, None, True) == (False, False, False) - - # --- SEC-13 · one enqueue is one bounded download ------------------------------------------- diff --git a/backend/tests/test_playable.py b/backend/tests/test_playable.py new file mode 100644 index 00000000..ddb58e35 --- /dev/null +++ b/backend/tests/test_playable.py @@ -0,0 +1,157 @@ +"""The phone-safe rule (`downloads.playable`): what a library file must be so a share link plays +on any phone or browser, and the one ffmpeg command that makes it so. + +The plan table is the measured device verdict, class by class: what failed on a real iPhone + iPad +(AV1, Matroska, HEVC, MP3 inside an .mp4) must change, and so must what browsers other than Safari +cannot be trusted with (10-bit / 4:4:4 H.264, VP9, Opus). The ffmpeg cases run the real encoder and +probe its output, because the rule is about the bytes, not the flags. +""" + +import shutil +import subprocess +from pathlib import Path + +import pytest + +from app.downloads import playable, probe + +# conftest turns a missing ffmpeg into a hard error in the gate (SIFTLODE_TEST_REQUIRE_DB), so this +# only ever skips on an explicit pure-logic run. +needs_ffmpeg = pytest.mark.skipif(shutil.which("ffmpeg") is None, reason="ffmpeg not available") + + +def _data(video=None, audio=None, fmt="mov,mp4,m4a,3gp,3g2,mj2", cover=False): + streams = [] + if cover: + streams.append( + {"codec_type": "video", "codec_name": "mjpeg", "disposition": {"attached_pic": 1}} + ) + if video: + codec, pix = video + streams.append({"codec_type": "video", "codec_name": codec, "pix_fmt": pix}) + if audio: + streams.append({"codec_type": "audio", "codec_name": audio}) + return {"streams": streams, "format": {"format_name": fmt}} + + +# (probe, extension) -> (ext, reencode_video, reencode_audio, needs_change) +CASES = { + "h264 8-bit + aac in mp4": (_data(("h264", "yuv420p"), "aac"), "mp4", ("mp4", 0, 0, 0)), + "full-range h264 is still 8-bit 4:2:0": ( + _data(("h264", "yuvj420p"), "aac"), + "mp4", + ("mp4", 0, 0, 0), + ), + "10-bit h264": (_data(("h264", "yuv420p10le"), "aac"), "mp4", ("mp4", 1, 0, 1)), + "4:4:4 h264": (_data(("h264", "yuv444p"), "aac"), "mp4", ("mp4", 1, 0, 1)), + "hevc (an iPhone's .mov)": (_data(("hevc", "yuv420p10le"), "aac"), "mov", ("mp4", 1, 0, 1)), + "vp9 + opus webm": (_data(("vp9", "yuv420p"), "opus"), "webm", ("mp4", 1, 1, 1)), + "av1": (_data(("av1", "yuv420p"), "aac"), "mp4", ("mp4", 1, 0, 1)), + "h264 + aac in mkv: remux only": (_data(("h264", "yuv420p"), "aac"), "mkv", ("mp4", 0, 0, 1)), + "h264 + opus in mp4": (_data(("h264", "yuv420p"), "opus"), "mp4", ("mp4", 0, 1, 1)), + "mp3 beside video in mp4": (_data(("h264", "yuv420p"), "mp3"), "mp4", ("mp4", 0, 1, 1)), + "a silent h264 video": (_data(("h264", "yuv420p")), "mp4", ("mp4", 0, 0, 0)), + "mp3": (_data(audio="mp3", fmt="mp3"), "mp3", ("mp3", 0, 0, 0)), + "mp3 with cover art": (_data(audio="mp3", fmt="mp3", cover=True), "mp3", ("mp3", 0, 0, 0)), + "mp3 inside an mp4: remux to .mp3": (_data(audio="mp3"), "mp4", ("mp3", 0, 0, 1)), + "aac in m4a": (_data(audio="aac"), "m4a", ("m4a", 0, 0, 0)), + "aac audio-only in mp4: remux to .m4a": (_data(audio="aac"), "mp4", ("m4a", 0, 0, 1)), + "opus in ogg": (_data(audio="opus", fmt="ogg"), "ogg", ("m4a", 0, 1, 1)), +} + + +@pytest.mark.parametrize("name", CASES) +def test_the_plan_for_each_class(name): + data, ext, (want_ext, rv, ra, needs) = CASES[name] + p = playable.plan(data, ext) + assert p is not None + assert (p.ext, p.reencode_video, p.reencode_audio, p.needs_change) == ( + want_ext, + bool(rv), + bool(ra), + bool(needs), + ) + + +@pytest.mark.parametrize( + "data", + [ + None, # ffprobe could not read it + {"streams": [], "format": {"format_name": "pdf"}}, + _data(("png", "rgb24"), fmt="png_pipe"), # a still image is a download-only file + ], +) +def test_a_file_that_is_not_media_has_no_plan(data): + assert playable.plan(data, "bin") is None + + +def test_the_extension_is_read_with_or_without_its_dot(): + data = _data(("h264", "yuv420p"), "aac") + assert playable.plan(data, ".MP4") == playable.plan(data, "mp4") + assert not playable.plan(data, ".MP4").needs_change + + +def test_a_video_re_encode_pins_8_bit_420_and_maps_only_the_judged_streams(): + p = playable.Plan("mp4", True, True, False, True) + cmd = playable.build_cmd(Path("/in.mov"), Path("/out.mp4"), p) + assert cmd[cmd.index("-pix_fmt") + 1] == "yuv420p" + assert cmd[cmd.index("-c:v") + 1] == "libx264" + assert cmd[cmd.index("-c:a") + 1] == "copy" + maps = [cmd[i + 1] for i, a in enumerate(cmd) if a == "-map"] + assert maps == ["0:V:0", "0:a:0?"] # never cover art, never a subtitle the muxer refuses + assert cmd[-3:] == ["-movflags", "+faststart", "/out.mp4"] + + +def test_an_audio_remux_copies_the_audio_alone(): + p = playable.Plan("mp3", False, False, False, True) + cmd = playable.build_cmd(Path("/in.mp4"), Path("/out.mp3"), p) + assert [cmd[i + 1] for i, a in enumerate(cmd) if a == "-map"] == ["0:a:0"] + assert "-c:v" not in cmd and "-pix_fmt" not in cmd + assert cmd[cmd.index("-c:a") + 1] == "copy" + assert "-movflags" not in cmd # an .mp3 has no moov atom to move + + +# --- the real encoder ------------------------------------------------------------------------ + + +def _ffmpeg(*args: str) -> None: + subprocess.run(["ffmpeg", "-y", "-loglevel", "error", *args], check=True) + + +def _convert(src: Path, tmp_path: Path) -> Path: + p = playable.plan(probe.probe_json(src), src.suffix) + assert p is not None and p.needs_change + dest = tmp_path / f"out.{p.ext}" + subprocess.run(playable.build_cmd(src, dest, p), check=True, capture_output=True) + return dest + + +@needs_ffmpeg +def test_a_10_bit_matroska_with_opus_comes_out_phone_safe(tmp_path): + src = tmp_path / "in.mkv" + _ffmpeg( + "-f", "lavfi", "-i", "testsrc2=size=160x120:rate=10:duration=1", + "-f", "lavfi", "-i", "sine=duration=1", + "-c:v", "libx264", "-pix_fmt", "yuv420p10le", "-c:a", "libopus", + str(src), + ) # fmt: skip + out = _convert(src, tmp_path) + data = probe.probe_json(out) + video, audio = probe.primary_streams(data) + assert (video["codec_name"], video["pix_fmt"], video["profile"]) == ("h264", "yuv420p", "High") + assert audio["codec_name"] == "aac" + assert out.suffix == ".mp4" + assert not playable.plan(data, out.suffix).needs_change # the rule accepts its own output + + +@needs_ffmpeg +def test_mp3_inside_an_mp4_comes_out_as_an_mp3(tmp_path): + # The one unsafe file prod held: an audio download remuxed into .mp4, which iPhone and iPad + # both refused to play. + src = tmp_path / "in.mp4" + _ffmpeg("-f", "lavfi", "-i", "sine=duration=1", "-c:a", "libmp3lame", "-f", "mp4", str(src)) + out = _convert(src, tmp_path) + data = probe.probe_json(out) + assert out.suffix == ".mp3" + assert probe.primary_streams(data)[1]["codec_name"] == "mp3" + assert not playable.plan(data, out.suffix).needs_change diff --git a/backend/tests/test_public_unlock_limit.py b/backend/tests/test_public_unlock_limit.py index 816575a2..d4fc12d6 100644 --- a/backend/tests/test_public_unlock_limit.py +++ b/backend/tests/test_public_unlock_limit.py @@ -20,7 +20,10 @@ _PW = "open sesame" @pytest.fixture def unlock_url(as_user, make_user, tmp_path, monkeypatch, upload_headers): monkeypatch.setattr(settings, "download_root", str(tmp_path)) - monkeypatch.setattr(probemod, "probe_media", lambda _p: ("h264", "aac", 320, 240, 5)) + video = {"codec_type": "video", "codec_name": "h264", "pix_fmt": "yuv420p"} + audio = {"codec_type": "audio", "codec_name": "aac"} + data = {"streams": [video, audio], "format": {"format_name": "mov,mp4", "duration": "5"}} + monkeypatch.setattr(probemod, "probe_json", lambda _p: data) owner = as_user(make_user()) job = owner.post( "/api/downloads/upload", content=b"fake-video", headers=upload_headers("clip.mp4") diff --git a/backend/tests/test_upload.py b/backend/tests/test_upload.py index 39de2ca2..db0699fd 100644 --- a/backend/tests/test_upload.py +++ b/backend/tests/test_upload.py @@ -7,15 +7,18 @@ guards that keep an upload out of paths that assume a re-fetchable source (edit DB-backed (real Postgres) — skipped when no database is reachable. `settings.download_root` is pointed at a per-test tmp dir so ingest's on-disk moves are isolated. The ingest/route tests -monkeypatch the shared `probe.probe_media` so they don't shell out to ffprobe; the classifier's OWN +monkeypatch the shared `probe.probe_json` so they don't shell out to ffprobe; the classifier's OWN parsing (image → file, cover-art skip, dims, duration) is covered directly by `TestProbeClassifier` -below, which stubs one level down (`run_ffprobe_json`) and exercises the real `probe_media`. +below, which stubs one level down (`run_ffprobe_json`) and exercises the real `probe_media`. The +phone-safe conversion of an upload runs the real ffmpeg (`TestPhoneSafeUpload`). """ import ast import asyncio import io import pathlib +import shutil +import subprocess from pathlib import Path import pytest @@ -25,9 +28,11 @@ from app.config import settings from app.routes._common import capped_stream_to from sqlalchemy import select -from app.downloads import mediakind, quota, storage +from app.downloads import gc as gcmod +from app.downloads import mediakind, pipeline, playable, quota, service, storage from app.downloads import probe as probemod from app.downloads import upload as uploadmod +from app.downloads.stated import StatedFailure from app.models import DISPLAY_TEXT_MAX, DownloadJob, DownloadQuota, MediaAsset @@ -51,9 +56,20 @@ def _root(tmp_path, monkeypatch): def _probe(monkeypatch, result): - # upload ingest now calls the shared `probe.probe_media` (same ground-truth ffprobe the download - # worker uses); patch it at that shared home. - monkeypatch.setattr(probemod, "probe_media", lambda _p: result) + """Stub the shared ffprobe call (`probe.probe_json`, the same ground truth the download worker + reads) with the JSON a file of these (vcodec, acodec, width, height, duration) would give — its + video 8-bit 4:2:0, so an H.264 + AAC .mp4 is already phone-safe and ingests straight to `done`. + All-None is a file ffprobe could not read.""" + v, a, w, h, d = result + streams = [] + if v: + streams.append( + {"codec_type": "video", "codec_name": v, "pix_fmt": "yuv420p", "width": w, "height": h} + ) + if a: + streams.append({"codec_type": "audio", "codec_name": a}) + data = {"streams": streams, "format": {"format_name": "mov,mp4", "duration": d}} + monkeypatch.setattr(probemod, "probe_json", lambda _p: data if streams else None) def test_ingest_generic_file_is_download_only(db, make_user, _root, monkeypatch): @@ -281,7 +297,7 @@ def test_upload_route_generic_forces_download( def test_upload_route_media_html_extension_not_served_as_html( - as_user, make_user, _root, monkeypatch, upload_headers + db, as_user, make_user, _root, monkeypatch, upload_headers ): """The XSS guard: a MEDIA-classified upload (real codecs) named with an executable extension must NOT be served with a guessed text/html type. The served Content-Type comes from the verified @@ -298,6 +314,14 @@ def test_upload_route_media_html_extension_not_served_as_html( assert r.status_code == 200, r.text job = r.json() assert job["asset"]["media_kind"] == "video" and job["asset"]["container"] == "html" + # Media in no phone-safe container is queued for conversion to .mp4, so today it is never served + # as .html at all. The serving guard below is the second line: a row from before that rule (or + # one whose conversion is skipped some day) must still not run as HTML. + assert job["status"] == "queued" + row = db.get(DownloadJob, job["id"]) + db.get(MediaAsset, row.asset_id).status = "ready" + row.status = "done" + db.commit() # Authenticated endpoint: never text/html. fr = c.get(f"/api/downloads/{job['id']}/file") @@ -1014,3 +1038,122 @@ class TestResolveMediaFacts: ) assert (v, a) == (None, "aac") assert mediakind.derive(v, a) == mediakind.AUDIO + + +# --- the phone-safe conversion (an iPhone's HEVC, a webm, an mkv) ------------------------------ + + +def _queued_upload(db, make_user, root, monkeypatch, name="clip.mov"): + """An upload no phone could play as it is: probed as HEVC, so ingest queues it.""" + _probe(monkeypatch, ("hevc", "aac", 640, 360, 6)) + src = root / "src.bin" + src.write_bytes(b"hevc bytes") + job = uploadmod.ingest_upload(db, make_user().id, filename=name, src_path=src, size=10) + return job, db.get(MediaAsset, job.asset_id) + + +class TestPhoneSafeUpload: + def test_an_unsafe_video_is_queued_for_the_worker_not_done( + self, db, make_user, _root, monkeypatch + ): + job, asset = _queued_upload(db, make_user, _root, monkeypatch) + assert (job.status, job.progress, asset.status) == ("queued", 0, "pending") + assert asset.rel_path.endswith(".mov") # the original waits at rel_path for the worker + assert storage.abs_path(str(_root), asset.rel_path).exists() + + def test_a_safe_video_still_lands_done(self, db, make_user, _root, monkeypatch): + _probe(monkeypatch, ("h264", "aac", 640, 360, 6)) + src = _root / "src.bin" + src.write_bytes(b"h264 bytes") + job = uploadmod.ingest_upload( + db, make_user().id, filename="clip.mp4", src_path=src, size=10 + ) + assert job.status == "done" and db.get(MediaAsset, job.asset_id).status == "ready" + + def test_deleting_a_queued_upload_removes_its_original(self, db, make_user, _root, monkeypatch): + job, asset = _queued_upload(db, make_user, _root, monkeypatch) + path, asset_id = storage.abs_path(str(_root), asset.rel_path), asset.id + service.delete(db, job) + assert not path.exists() + assert db.get(MediaAsset, asset_id) is None + + def test_one_cancelled_mid_conversion_is_reaped_once_the_worker_let_go( + self, db, make_user, _root, monkeypatch + ): + job, asset = _queued_upload(db, make_user, _root, monkeypatch) + path, asset_id = storage.abs_path(str(_root), asset.rel_path), asset.id + asset.status = "downloading" # the worker child is reading the original + job.status = "running" + db.commit() + service.cancel(db, job) + assert path.exists() # never pulled out from under a running ffmpeg + db.get(MediaAsset, asset_id).status = "pending" # the worker stopped the child and let go + db.commit() + gcmod.run_download_gc(db) + db.expire_all() + assert not path.exists() + assert db.get(MediaAsset, asset_id) is None + + def test_a_failed_conversion_keeps_the_original_for_retry( + self, db, make_user, _root, monkeypatch + ): + job, asset = _queued_upload(db, make_user, _root, monkeypatch) + path = storage.abs_path(str(_root), asset.rel_path) + job.status, asset.status = "running", "downloading" + db.commit() + + def _ffmpeg_fails(*_a, **_k): + raise StatedFailure("ffmpeg failed (1): unsupported") + + monkeypatch.setattr(pipeline.editmod, "run_ffmpeg", _ffmpeg_fails) + pipeline._convert_upload(job.id, asset.id) + db.expire_all() + assert (db.get(DownloadJob, job.id).status, db.get(MediaAsset, asset.id).status) == ( + "error", + "error", + ) + assert path.exists() + service.resume(db, db.get(DownloadJob, job.id)) + assert db.get(MediaAsset, asset.id).status == "pending" # Retry converts from it again + + @pytest.mark.skipif(shutil.which("ffmpeg") is None, reason="ffmpeg not available") + def test_the_worker_converts_it_and_keeps_only_the_safe_file( + self, db, make_user, _root, monkeypatch + ): + # Real bytes, real probe, real ffmpeg: a 10-bit H.264 + Opus Matroska, the kind of file + # that reached the share page as-is before this rule. + src = _root / "src.mkv" + subprocess.run( + [ + "ffmpeg", "-y", "-loglevel", "error", + "-f", "lavfi", "-i", "testsrc2=size=160x120:rate=10:duration=1", + "-f", "lavfi", "-i", "sine=duration=1", + "-c:v", "libx264", "-pix_fmt", "yuv420p10le", "-c:a", "libopus", str(src), + ], + check=True, + ) # fmt: skip + job = uploadmod.ingest_upload( + db, make_user().id, filename="holiday.mkv", src_path=src, size=src.stat().st_size + ) + asset = db.get(MediaAsset, job.asset_id) + original = storage.abs_path(str(_root), asset.rel_path) + assert job.status == "queued" + job.status, asset.status = "running", "downloading" # what the worker's claim does + db.commit() + + pipeline._convert_upload(job.id, asset.id) + + db.expire_all() + asset = db.get(MediaAsset, job.asset_id) + assert db.get(DownloadJob, job.id).status == "done" + assert (asset.status, asset.container, asset.vcodec, asset.acodec) == ( + "ready", + "mp4", + "h264", + "aac", + ) + final = storage.abs_path(str(_root), asset.rel_path) + assert final.suffix == ".mp4" and final.exists() + assert asset.size_bytes == final.stat().st_size + assert not original.exists() # one file per upload + assert not playable.plan(probemod.probe_json(final), final.suffix).needs_change diff --git a/frontend/src/components/UploadDialog.dom.test.tsx b/frontend/src/components/UploadDialog.dom.test.tsx index 4a3ca217..097d0561 100644 --- a/frontend/src/components/UploadDialog.dom.test.tsx +++ b/frontend/src/components/UploadDialog.dom.test.tsx @@ -10,7 +10,7 @@ import { render, screen, fireEvent, waitFor, cleanup } from "@testing-library/re import { QueryClient, QueryClientProvider } from "@tanstack/react-query"; const h = vi.hoisted(() => ({ - uploadDownload: vi.fn(async (_f: File, _o: unknown) => ({ id: 42 })), + uploadDownload: vi.fn(async (_f: File, _o: unknown) => ({ id: 42, status: "done" })), // The dialog reads the per-file upload cap so it can refuse an over-cap file before spending the // user's bandwidth on it. Generous by default; the cap test below lowers it. downloadUsage: vi.fn(async () => ({ upload_max_bytes: 1_000_000 })), @@ -78,9 +78,25 @@ describe("UploadDialog", () => { expect(passedFile).toBe(f); expect((opts as { displayName?: string }).displayName).toBe("My notes"); await waitFor(() => expect(h.notify).toHaveBeenCalledTimes(1)); + expect(h.notify.mock.calls[0]![0]).toMatchObject({ message: "downloads.upload.added" }); await waitFor(() => expect(onClose).toHaveBeenCalled()); }); + it("says so when the server queued the upload for its phone-safe conversion", async () => { + // A video no phone could play as it is (an iPhone's HEVC .mov) comes back `queued`, not `done`: + // "File uploaded" alone would send the user to a Library row that is still converting. + h.uploadDownload.mockResolvedValueOnce({ id: 43, status: "queued" }); + wrap(); + fireEvent.change(fileInput(), { + target: { files: [new File(["x"], "clip.mov", { type: "video/quicktime" })] }, + }); + fireEvent.click(screen.getByText("downloads.upload.submit")); + await waitFor(() => expect(h.notify).toHaveBeenCalledTimes(1)); + expect(h.notify.mock.calls[0]![0]).toMatchObject({ + message: "downloads.upload.addedConverting", + }); + }); + it("keeps Upload disabled until a file is chosen", () => { wrap(); const btn = screen.getByText("downloads.upload.submit").closest("button")!; diff --git a/frontend/src/components/UploadDialog.tsx b/frontend/src/components/UploadDialog.tsx index d410e70e..414624e4 100644 --- a/frontend/src/components/UploadDialog.tsx +++ b/frontend/src/components/UploadDialog.tsx @@ -19,7 +19,14 @@ import { Input } from "./ui/Input"; // Local-file ingest. A general upload — ANY file type, no format list: media // (video/audio) joins the library with the full function set, a generic file is download-only. The // file is streamed to the server via XHR so this dialog can show real upload progress. On success it -// lands in the Library as an already-`done` job, so we invalidate + point the user there. +// lands in the Library as an already-`done` job — or, for media no phone could play as it is (an +// iPhone's HEVC, a webm, an mkv), as a `queued` job the worker converts first — so we invalidate, +// say which, and point the user there. +// Out here, not inline in the upload handler: a `?:` inside its `try` makes the React Compiler +// skip the whole dialog. +const uploadedKey = (status: string) => + status === "done" ? "downloads.upload.added" : "downloads.upload.addedConverting"; + export default function UploadDialog({ onClose }: { onClose: () => void }) { const { t, i18n } = useTranslation(); const lang = i18n.language; @@ -73,7 +80,7 @@ export default function UploadDialog({ onClose }: { onClose: () => void }) { setPct(0); const displayName = name.trim() || undefined; try { - await api.uploadDownload(file, { + const job = await api.uploadDownload(file, { displayName, onProgress: setPct, signal: ac.signal, @@ -81,7 +88,7 @@ export default function UploadDialog({ onClose }: { onClose: () => void }) { invalidateDownloads(qc); notify({ level: "success", - message: t("downloads.upload.added"), + message: t(uploadedKey(job.status)), action: { label: t("downloads.dialog.view"), onClick: () => navigateTo("downloads") }, }); onClose(); diff --git a/frontend/src/i18n/locales/en/downloads.json b/frontend/src/i18n/locales/en/downloads.json index a385c9ce..95684f65 100644 --- a/frontend/src/i18n/locales/en/downloads.json +++ b/frontend/src/i18n/locales/en/downloads.json @@ -36,6 +36,7 @@ "uploading": "Uploading… {{pct}}%", "uploadProgress": "Upload progress", "added": "File uploaded", + "addedConverting": "File uploaded — converting it so it plays on any phone", "failed": "Couldn't upload the file" }, "tabs": { diff --git a/frontend/src/i18n/locales/hu/downloads.json b/frontend/src/i18n/locales/hu/downloads.json index 0e9c92e9..8371a869 100644 --- a/frontend/src/i18n/locales/hu/downloads.json +++ b/frontend/src/i18n/locales/hu/downloads.json @@ -36,6 +36,7 @@ "uploading": "Feltöltés… {{pct}}%", "uploadProgress": "Feltöltés folyamata", "added": "Fájl feltöltve", + "addedConverting": "Fájl feltöltve — átalakítjuk, hogy minden telefonon lejátszható legyen", "failed": "Nem sikerült feltölteni a fájlt" }, "tabs": { diff --git a/frontend/src/lib/api/downloads.ts b/frontend/src/lib/api/downloads.ts index 2c05db1d..fcac578d 100644 --- a/frontend/src/lib/api/downloads.ts +++ b/frontend/src/lib/api/downloads.ts @@ -196,7 +196,8 @@ export const downloadsApi = { }): Promise => req("/api/downloads", { method: "POST", body: JSON.stringify(body) }), // Ingest a local file into the library. Sent via XHR rather than `req` so the dialog can // show upload progress; any file type is accepted (media gets the full function set, a generic - // file is download-only). Returns the already-`done` job the ingest creates. + // file is download-only). Returns the job the ingest creates: `done`, or `queued` while the + // worker converts media no phone could play as it is (`downloads/upload.py`). // The body IS the file, and the two names ride as headers. Not a style choice: a multipart part is // a DECLARED body, which FastAPI reads and spools BEFORE the route's `current_user` dependency // runs — so with the old shape an unauthenticated caller could make the server write the whole From e886890d6e1ae16df439c0403df4bec91e5ca00a Mon Sep 17 00:00:00 2001 From: npeter83 Date: Mon, 5 Oct 2026 20:51:39 +0200 Subject: [PATCH 03/12] perf(watch): a shared link loads the player, not the app MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit A /watch link is opened by whoever it was sent to — signed out, often on a phone on mobile data — and it booted through main.tsx: every app provider, the app's i18n (every locale of every area), the flag sprites, ~392 KB gzip before the player could start. It now has its own HTML entry, watch.html → watchMain.tsx, which imports WatchPage and what it renders; the server hands every /watch/ path that page (the OG block injected as before), and vite dev does the same. Measured on the build: /watch JS 290 → 87 KB gzip, CSS 104 → 19 KB; the app's own bytes unchanged. Two leaks had to go for that: lib/format imported the app's i18n for one "just now" string (the relative-time formatters now take `t`, like the file's other t-formatters), and the boundary's crash copy came from the same i18n (CrashBoundary takes its copy; ErrorBoundary is the app's wrapper, the watch entry gives it the page's own strings). The page's popover tree went too: it renders no popover. Held by: a contract test walking the entry's runtime imports, naming the path to anything only the app may load (red on a planted i18n import in lib/format), and holding watch.html's head equal to index.html's; a DB-lane test that every /watch/ path gets the player's page with the link's OG tags and app paths get the app (red on the old routing). Screenshots at 375 px, EN and HU, video / audio / password / gone / wrong password / unlocked: identical to the old entry's, bar the video's buffered bar. --- CLAUDE.md | 3 +- backend/app/downloads/og.py | 9 +- backend/app/main.py | 16 ++- backend/tests/test_spa_entries.py | 67 +++++++++++ .../src/components/AdminUsers.dom.test.tsx | 2 +- frontend/src/components/AdminUsers.tsx | 2 +- frontend/src/components/CrashBoundary.tsx | 62 ++++++++++ frontend/src/components/ErrorBoundary.tsx | 79 ++++--------- frontend/src/components/Home.tsx | 2 +- frontend/src/components/PlayerModal.tsx | 4 +- frontend/src/components/Scheduler.tsx | 2 +- frontend/src/components/VideoCard.tsx | 2 +- frontend/src/components/WatchPage.tsx | 73 +----------- frontend/src/components/downloadRow.tsx | 2 +- .../components/ui/RelativeTime.dom.test.tsx | 4 +- frontend/src/components/ui/RelativeTime.tsx | 5 +- frontend/src/components/watchStrings.ts | 78 +++++++++++++ frontend/src/lib/format.test.ts | 27 ++--- frontend/src/lib/format.ts | 13 ++- frontend/src/main.tsx | 26 ++--- frontend/src/watchEntry.contract.test.ts | 109 ++++++++++++++++++ frontend/src/watchMain.tsx | 26 +++++ frontend/vite.config.ts | 19 ++- frontend/watch.html | 35 ++++++ 24 files changed, 483 insertions(+), 184 deletions(-) create mode 100644 backend/tests/test_spa_entries.py create mode 100644 frontend/src/components/CrashBoundary.tsx create mode 100644 frontend/src/components/watchStrings.ts create mode 100644 frontend/src/watchEntry.contract.test.ts create mode 100644 frontend/src/watchMain.tsx create mode 100644 frontend/watch.html diff --git a/CLAUDE.md b/CLAUDE.md index 87ef68f4..b5d30889 100644 --- a/CLAUDE.md +++ b/CLAUDE.md @@ -65,7 +65,8 @@ how it gets built. Concretely: `downloads/`, `youtube/`, `issues/` (shared by routes + scheduler + worker) → flat core (`auth`, `quota`, `sysconfig`, `state`, `audit`, `notifications`, `userscope`, `ratelimit`, `bodylimit`, `progress`, `collation`, `security`, `logs`, `logsink`, `diagnostics`, `debugsession`) → `models/db/config`. **Dependency direction is one-way: core never imports routes.** -Frontend: `main.tsx` provider tree → `App.tsx` shell → `moduleRegistry` route-chunks → module +Frontend: `main.tsx` provider tree → `App.tsx` shell (a shared `/watch` link boots from its own +entry, `watch.html` → `watchMain.tsx`, held app-free by a contract test) → `moduleRegistry` route-chunks → module components → the **platform layer `components/ui/`** (domain-free primitives: the popover engine and its menus, `Modal`/`Overlay`, `Button`/`form`, `DataTable`/`VirtualGrid`, `QueryState`, …). That boundary is one-way and **lint-enforced**, not just described: a platform component may not import a diff --git a/backend/app/downloads/og.py b/backend/app/downloads/og.py index 29d1e9aa..2bc7bf47 100644 --- a/backend/app/downloads/og.py +++ b/backend/app/downloads/og.py @@ -147,13 +147,14 @@ def _watch_tags(token: str, db: Session) -> str | None: return "\n ".join(tags) -def render_watch_html(token: str, index_html: Path, db: Session) -> str | None: - """Return index.html with the OG block replaced for this watch token, or None to serve the - generic page unchanged (invalid/expired/not-ready link, or a template without the markers).""" +def render_watch_html(token: str, page_html: Path, db: Session) -> str | None: + """Return the watch page (watch.html) with the OG block replaced for this token, or None to + serve the generic page unchanged (invalid/expired/not-ready link, or a template without the + markers).""" tags = _watch_tags(token, db) if tags is None: return None - text = index_html.read_text(encoding="utf-8") + text = page_html.read_text(encoding="utf-8") if "" not in text: return None # A function replacement avoids re.sub interpreting backslashes/group refs in `tags`. diff --git a/backend/app/main.py b/backend/app/main.py index 14a2dafa..e06746b9 100644 --- a/backend/app/main.py +++ b/backend/app/main.py @@ -339,6 +339,8 @@ STATIC_DIR = Path(__file__).parent / "static_spa" # stay cached but forces revalidation (cheap 304 via the FileResponse ETag) on every load. The # hashed bundles under /assets can cache forever (a new build changes their filename). INDEX_HTML = STATIC_DIR / "index.html" +# A public share link's own page: the player alone, without the app's bundle (frontend/watch.html). +WATCH_HTML = STATIC_DIR / "watch.html" INDEX_HEADERS = {"Cache-Control": "no-cache"} app.mount( "/assets", @@ -354,16 +356,16 @@ async def index() -> FileResponse: @app.get("/watch/{token}") async def watch_page(token: str): - """Serve the SPA for a public share link, but with per-video Open Graph tags injected so the + """Serve the public player page for a share link, with per-video Open Graph tags injected so the link unfurls richly (title/channel/thumbnail) in Messenger/social crawlers. A real browser - ignores the extra tags and hydrates WatchPage as usual; invalid/expired/password links fall - back to the generic card.""" + ignores the extra tags and boots WatchPage; invalid/expired/password links fall back to the + generic card.""" from app.downloads import og with SessionLocal() as db: - rendered = og.render_watch_html(token, INDEX_HTML, db) + rendered = og.render_watch_html(token, WATCH_HTML, db) if rendered is None: - return FileResponse(INDEX_HTML, headers=INDEX_HEADERS) + return FileResponse(WATCH_HTML, headers=INDEX_HEADERS) return HTMLResponse(rendered, headers=INDEX_HEADERS) @@ -381,4 +383,8 @@ async def spa_fallback(full_path: str) -> FileResponse: # Real SPA-root assets (welcome images, favicon, robots.txt) rarely change and have # stable names, so a moderate cache is safe and satisfies the cache-lifetime audit. return FileResponse(candidate, headers={"Cache-Control": "public, max-age=604800"}) + # A /watch/ path the route above does not match (a trailing segment, no token) is still the + # player's: WatchPage reads the token off the path and says when a link is gone. + if full_path.startswith("watch/"): + return FileResponse(WATCH_HTML, headers=INDEX_HEADERS) return FileResponse(INDEX_HTML, headers=INDEX_HEADERS) diff --git a/backend/tests/test_spa_entries.py b/backend/tests/test_spa_entries.py new file mode 100644 index 00000000..7fab4d24 --- /dev/null +++ b/backend/tests/test_spa_entries.py @@ -0,0 +1,67 @@ +"""Which HTML page the server hands a path (main.py), DB-lane. + +The frontend builds two entries: the app (index.html) and a shared link's player alone (watch.html), so +the friend a /watch link was sent to never downloads the app. Every /watch/ path must get the player's +page — with the link's Open Graph tags when the token names a playable link — and nothing else may. +""" + +import pytest + +from app import main +from app.config import settings +from app.downloads import probe as probemod + +_PAGE = """ + +""" + + +@pytest.fixture +def pages(tmp_path, monkeypatch): + index, watch = tmp_path / "index.html", tmp_path / "watch.html" + index.write_text(_PAGE.format(entry="app"), encoding="utf-8") + watch.write_text(_PAGE.format(entry="watch"), encoding="utf-8") + monkeypatch.setattr(main, "INDEX_HTML", index) + monkeypatch.setattr(main, "WATCH_HTML", watch) + + +@pytest.fixture +def link_token(as_user, make_user, tmp_path, monkeypatch, upload_headers): + monkeypatch.setattr(settings, "download_root", str(tmp_path / "media")) + video = {"codec_type": "video", "codec_name": "h264", "pix_fmt": "yuv420p"} + audio = {"codec_type": "audio", "codec_name": "aac"} + data = {"streams": [video, audio], "format": {"format_name": "mov,mp4", "duration": "5"}} + monkeypatch.setattr(probemod, "probe_json", lambda _p: data) + owner = as_user(make_user()) + job = owner.post( + "/api/downloads/upload", + content=b"fake-video", + headers=upload_headers("clip.mp4", display_name="Holiday clip"), + ) + assert job.status_code == 200, job.text + link = owner.post(f"/api/downloads/{job.json()['id']}/links", json={}) + assert link.status_code == 200, link.text + return link.json()["token"] + + +@pytest.mark.parametrize("path", ["/", "/feed", "/downloads/42"]) +def test_an_app_path_gets_the_app(pages, client, path): + r = client.get(path) + assert r.status_code == 200 + assert "/assets/app.js" in r.text + + +@pytest.mark.parametrize("path", ["/watch/no-such-token", "/watch/no-such-token/extra"]) +def test_every_watch_path_gets_the_player_alone(pages, client, path): + r = client.get(path) + assert r.status_code == 200 + assert "/assets/watch.js" in r.text and "/assets/app.js" not in r.text + assert r.headers["cache-control"] == "no-cache" + + +def test_a_live_link_unfurls_from_the_players_page(pages, client, link_token): + r = client.get(f"/watch/{link_token}") + assert r.status_code == 200 + assert "/assets/watch.js" in r.text + assert 'content="Holiday clip"' in r.text + assert '' not in r.text # replaced, not appended diff --git a/frontend/src/components/AdminUsers.dom.test.tsx b/frontend/src/components/AdminUsers.dom.test.tsx index ef024bcf..7dc77a69 100644 --- a/frontend/src/components/AdminUsers.dom.test.tsx +++ b/frontend/src/components/AdminUsers.dom.test.tsx @@ -91,7 +91,7 @@ describe("the last sign-in column", () => { , ); - expect(await screen.findByText(relativeTime(at, "en"))).toBeTruthy(); + expect(await screen.findByText(relativeTime(at, "en", i18n.t))).toBeTruthy(); expect(screen.getAllByText("—")).toHaveLength(1); }); }); diff --git a/frontend/src/components/AdminUsers.tsx b/frontend/src/components/AdminUsers.tsx index da8953c7..5f8d3689 100644 --- a/frontend/src/components/AdminUsers.tsx +++ b/frontend/src/components/AdminUsers.tsx @@ -316,7 +316,7 @@ function UsersRoles({ me }: { me: Me }) { } > - {u.last_login_at ? relativeTime(u.last_login_at, lang) : "—"} + {u.last_login_at ? relativeTime(u.last_login_at, lang, t) : "—"} CrashCopy; + /** What the entry does beyond the log line (the app keeps a fatal notice in its center). */ + onCrash?: () => void; +} +interface State { + crashed: boolean; +} + +// Catches render-time crashes anywhere in the tree, logs them and offers a reload. Free of the app's +// i18n and notification store, so the public /watch entry wraps its page in it without loading +// either; `ErrorBoundary` is the app's wrapper around it. +export class CrashBoundary extends Component { + state: State = { crashed: false }; + + static getDerivedStateFromError(): State { + return { crashed: true }; + } + + componentDidCatch(error: Error, info: ErrorInfo) { + // The tab is already reloading onto a newer build (lib/staleBuild): this is the chunk it could not + // load, not a crash — no fatal notice, which would outlive the reload. + if (reloadingForNewBuild()) return; + log.fatal("ui.crash", "the app crashed while rendering", { + ...errorAttrs(error), + ...(info.componentStack ? { component_stack: info.componentStack } : {}), + }); + this.props.onCrash?.(); + } + + render() { + if (this.state.crashed) { + if (reloadingForNewBuild()) return null; + const copy = this.props.copy(); + return ( +
+
+
{copy.title}
+
{copy.subtitle}
+ +
+
+ ); + } + return this.props.children; + } +} diff --git a/frontend/src/components/ErrorBoundary.tsx b/frontend/src/components/ErrorBoundary.tsx index ce94e769..727e808d 100644 --- a/frontend/src/components/ErrorBoundary.tsx +++ b/frontend/src/components/ErrorBoundary.tsx @@ -1,61 +1,30 @@ -import { Component, type ErrorInfo, type ReactNode } from "react"; +import type { ReactNode } from "react"; import i18n from "../i18n"; import { notify } from "../lib/notifications"; -import { errorAttrs, log } from "../lib/log"; -import { reloadingForNewBuild } from "../lib/staleBuild"; -import { Button } from "./ui/Button"; +import { CrashBoundary, type CrashCopy } from "./CrashBoundary"; -interface Props { - children: ReactNode; -} -interface State { - crashed: boolean; -} +const copy = (): CrashCopy => ({ + title: i18n.t("errors.boundary.title"), + subtitle: i18n.t("errors.boundary.subtitle"), + reload: i18n.t("errors.boundary.reload"), +}); -// Catches render-time crashes anywhere in the tree, logs a fatal notification (kept -// in the center across the reload) and offers a recovery action. -export default class ErrorBoundary extends Component { - state: State = { crashed: false }; +// A fatal notification, kept in the center across the reload. +const onCrash = () => + notify({ + level: "fatal", + title: i18n.t("errors.boundary.notifTitle"), + // Never the raw `error.message`: a JS engine's English ("Cannot read properties of undefined…") + // is no message for the reader — the `ui.crash` log carries it for diagnosis. + message: i18n.t("errors.boundary.notifMessage"), + requiresInteraction: true, + }); - static getDerivedStateFromError(): State { - return { crashed: true }; - } - - componentDidCatch(error: Error, info: ErrorInfo) { - // The tab is already reloading onto a newer build (lib/staleBuild): this is the chunk it could not - // load, not a crash — no fatal notice, which would outlive the reload. - if (reloadingForNewBuild()) return; - log.fatal("ui.crash", "the app crashed while rendering", { - ...errorAttrs(error), - ...(info.componentStack ? { component_stack: info.componentStack } : {}), - }); - notify({ - level: "fatal", - title: i18n.t("errors.boundary.notifTitle"), - // Never the raw `error.message`: a JS engine's English ("Cannot read properties of undefined…") - // is no message for the reader — the `ui.crash` log above carries it for diagnosis. - message: i18n.t("errors.boundary.notifMessage"), - requiresInteraction: true, - }); - } - - render() { - if (this.state.crashed) { - if (reloadingForNewBuild()) return null; - return ( -
-
-
- {i18n.t("errors.boundary.title")} -
-
{i18n.t("errors.boundary.subtitle")}
- -
-
- ); - } - return this.props.children; - } +// The app's crash boundary: `CrashBoundary` in the app's language, plus the notice in the center. +export default function ErrorBoundary({ children }: { children: ReactNode }) { + return ( + + {children} + + ); } diff --git a/frontend/src/components/Home.tsx b/frontend/src/components/Home.tsx index 3d68b850..cd73a7ba 100644 --- a/frontend/src/components/Home.tsx +++ b/frontend/src/components/Home.tsx @@ -546,7 +546,7 @@ function Hero({ ? // The channel is the subtitle already. [ video.view_count != null && `${formatCompact(video.view_count, lang)} ${t("card.views")}`, - relativeTime(video.published_at, lang), + relativeTime(video.published_at, lang, t), ] : []; const factLine = facts.filter(Boolean).join(" · "); diff --git a/frontend/src/components/PlayerModal.tsx b/frontend/src/components/PlayerModal.tsx index 3736cbf1..e6506986 100644 --- a/frontend/src/components/PlayerModal.tsx +++ b/frontend/src/components/PlayerModal.tsx @@ -977,7 +977,7 @@ export default function PlayerModal({ )} {metaSep} - {relativeTime(active.published_at, lang)} + {relativeTime(active.published_at, lang, t)} {active.published_at && ` · ${fullDate(active.published_at)}`} {active.duration_seconds != null && ( @@ -1009,7 +1009,7 @@ export default function PlayerModal({ {detail.data?.published_at && ( {metaSep} - {relativeTime(detail.data.published_at, lang)} ·{" "} + {relativeTime(detail.data.published_at, lang, t)} ·{" "} {fullDate(detail.data.published_at)} )} diff --git a/frontend/src/components/Scheduler.tsx b/frontend/src/components/Scheduler.tsx index 0d8a1967..29075f6b 100644 --- a/frontend/src/components/Scheduler.tsx +++ b/frontend/src/components/Scheduler.tsx @@ -254,7 +254,7 @@ function JobRow({ {job.running ? t("scheduler.runningNow") : job.last_finished - ? t("scheduler.lastRun", { time: relativeTime(job.last_finished, lang) }) + ? t("scheduler.lastRun", { time: relativeTime(job.last_finished, lang, t) }) : t("scheduler.neverRun")} {job.last_error ? ( diff --git a/frontend/src/components/VideoCard.tsx b/frontend/src/components/VideoCard.tsx index d8f624e1..5aa4a335 100644 --- a/frontend/src/components/VideoCard.tsx +++ b/frontend/src/components/VideoCard.tsx @@ -420,7 +420,7 @@ function VideoCard({ ); const published = ( <> - {relativeTime(video.published_at, lang)} + {relativeTime(video.published_at, lang, t)} {video.published_at && ( <> {" · "} diff --git a/frontend/src/components/WatchPage.tsx b/frontend/src/components/WatchPage.tsx index 40da9bf0..355cb66d 100644 --- a/frontend/src/components/WatchPage.tsx +++ b/frontend/src/components/WatchPage.tsx @@ -9,10 +9,11 @@ import { send } from "../lib/http"; import { Input } from "./ui/Input"; import { SURFACE } from "./ui/surfaces"; import { LinkButton } from "./ui/LinkButton"; +import { T, lang } from "./watchStrings"; -// Standalone, login-free video page for a shared /watch/ link. Rendered OUTSIDE the app -// tree (no auth, no providers) — like the /privacy and /terms pages. Uses plain fetch and a tiny -// self-contained i18n dict (the public viewer has no app language preference). +// Standalone, login-free video page for a shared /watch/ link. Its own entry (watchMain.tsx, +// watch.html): no auth, no providers, none of the app's bundle. Uses plain fetch and a tiny +// self-contained i18n dict (`watchStrings`: the public viewer has no app language preference). // Advertise the playing video to the OS via the Media Session API: lock-screen / control-center // metadata + play/pause handlers. This is also what lets a browser's own "background playback" @@ -75,72 +76,6 @@ function displayUrl(url: string): string { } } -const STR = { - en: { - loading: "Loading…", - gone: "This link is no longer available.", - goneHint: "It may have expired or been revoked by the owner.", - passwordTitle: "This video is password-protected", - passwordPlaceholder: "Enter password", - watch: "Watch", - wrong: "Wrong password.", - download: "Download", - file: "File", - links: "Links", - brand: "Shared via Siftlode", - repeat: "Repeat", - failed: "Couldn't load this link.", - failedHint: "Check your connection and try again.", - retry: "Try again", - tooMany: "Too many attempts. Try again in a few minutes.", - unlockFailed: "Couldn't check the password. Try again.", - }, - hu: { - loading: "Betöltés…", - gone: "Ez a link már nem elérhető.", - goneHint: "Lehet, hogy lejárt, vagy a tulajdonos visszavonta.", - passwordTitle: "Ez a videó jelszóval védett", - passwordPlaceholder: "Add meg a jelszót", - watch: "Megnézem", - wrong: "Hibás jelszó.", - download: "Letöltés", - file: "Fájl", - links: "Linkek", - brand: "Megosztva a Siftlode-dal", - repeat: "Ismétlés", - failed: "Nem sikerült betölteni a linket.", - failedHint: "Ellenőrizd a kapcsolatot, és próbáld újra.", - retry: "Újrapróbálás", - tooMany: "Túl sok próbálkozás. Próbáld újra pár perc múlva.", - unlockFailed: "Nem sikerült ellenőrizni a jelszót. Próbáld újra.", - }, - de: { - loading: "Wird geladen…", - gone: "Dieser Link ist nicht mehr verfügbar.", - goneHint: "Er ist möglicherweise abgelaufen oder wurde widerrufen.", - passwordTitle: "Dieses Video ist passwortgeschützt", - passwordPlaceholder: "Passwort eingeben", - watch: "Ansehen", - wrong: "Falsches Passwort.", - download: "Herunterladen", - file: "Datei", - links: "Links", - brand: "Geteilt über Siftlode", - repeat: "Wiederholen", - failed: "Der Link konnte nicht geladen werden.", - failedHint: "Prüfe deine Verbindung und versuche es erneut.", - retry: "Erneut versuchen", - tooMany: "Zu viele Versuche. Versuche es in ein paar Minuten erneut.", - unlockFailed: "Das Passwort konnte nicht geprüft werden. Versuche es erneut.", - }, -} as const; - -const lang = - ((navigator.language || "en").slice(0, 2) as keyof typeof STR) in STR - ? ((navigator.language || "en").slice(0, 2) as keyof typeof STR) - : "en"; -const T = STR[lang]; - export default function WatchPage() { const token = window.location.pathname.split("/watch/")[1]?.split(/[/?#]/)[0] ?? ""; // Seeded from the URL rather than corrected by the effect below: with no token there is nothing diff --git a/frontend/src/components/downloadRow.tsx b/frontend/src/components/downloadRow.tsx index 45096d94..9809a525 100644 --- a/frontend/src/components/downloadRow.tsx +++ b/frontend/src/components/downloadRow.tsx @@ -304,7 +304,7 @@ export function DownloadRow({ {" · "} {" "} - {relativeTime(job.created_at, lang)} + {relativeTime(job.created_at, lang, t)} ) : null} {job.status === "error" ? ( diff --git a/frontend/src/components/ui/RelativeTime.dom.test.tsx b/frontend/src/components/ui/RelativeTime.dom.test.tsx index ff8ff74c..33b6d3eb 100644 --- a/frontend/src/components/ui/RelativeTime.dom.test.tsx +++ b/frontend/src/components/ui/RelativeTime.dom.test.tsx @@ -4,7 +4,7 @@ import { afterEach, describe, expect, it } from "vitest"; import { cleanup, render } from "@testing-library/react"; import { formatDateTime, formatDateTimeSec, relativeTime } from "../../lib/format"; -import "../../i18n"; // the real instance, in English: the component names time in the UI language +import i18n from "../../i18n"; // the real instance, in English: the component names time in the UI language import { RelativeTime } from "./RelativeTime"; afterEach(cleanup); @@ -15,7 +15,7 @@ describe("RelativeTime", () => { it("says how long ago, and names the exact minute on hover", () => { const { container } = render(); const el = container.querySelector("time")!; - expect(el.textContent).toBe(relativeTime(THREE_DAYS_AGO, "en")); + expect(el.textContent).toBe(relativeTime(THREE_DAYS_AGO, "en", i18n.t)); expect(el.getAttribute("title")).toBe(formatDateTime(THREE_DAYS_AGO, "en")); expect(el.getAttribute("dateTime")).toBe(THREE_DAYS_AGO); }); diff --git a/frontend/src/components/ui/RelativeTime.tsx b/frontend/src/components/ui/RelativeTime.tsx index 2a01b70d..58586a79 100644 --- a/frontend/src/components/ui/RelativeTime.tsx +++ b/frontend/src/components/ui/RelativeTime.tsx @@ -17,9 +17,10 @@ export function RelativeTime({ empty?: string; className?: string; }) { - const lang = useTranslation().i18n.language; + const { t, i18n } = useTranslation(); + const lang = i18n.language; const ms = typeof at === "number" ? at : at ? new Date(at).getTime() : NaN; - const text = Number.isFinite(ms) ? relativeFromMs(ms, lang) : ""; + const text = Number.isFinite(ms) ? relativeFromMs(ms, lang, t) : ""; if (!text) return empty ? {empty} : null; const iso = new Date(ms).toISOString(); return ( diff --git a/frontend/src/components/watchStrings.ts b/frontend/src/components/watchStrings.ts new file mode 100644 index 00000000..18ab3c88 --- /dev/null +++ b/frontend/src/components/watchStrings.ts @@ -0,0 +1,78 @@ +// The public /watch page's own strings. The page is opened by whoever got the link, signed out and +// with no app language preference, so it reads the browser's language against this small dict +// instead of loading the app's i18n (every locale of every area — half of what the page weighed). + +const STR = { + en: { + loading: "Loading…", + gone: "This link is no longer available.", + goneHint: "It may have expired or been revoked by the owner.", + passwordTitle: "This video is password-protected", + passwordPlaceholder: "Enter password", + watch: "Watch", + wrong: "Wrong password.", + download: "Download", + file: "File", + links: "Links", + brand: "Shared via Siftlode", + repeat: "Repeat", + failed: "Couldn't load this link.", + failedHint: "Check your connection and try again.", + retry: "Try again", + tooMany: "Too many attempts. Try again in a few minutes.", + unlockFailed: "Couldn't check the password. Try again.", + crashTitle: "Something went wrong.", + crashHint: "Reload the page to try again.", + reload: "Reload", + }, + hu: { + loading: "Betöltés…", + gone: "Ez a link már nem elérhető.", + goneHint: "Lehet, hogy lejárt, vagy a tulajdonos visszavonta.", + passwordTitle: "Ez a videó jelszóval védett", + passwordPlaceholder: "Add meg a jelszót", + watch: "Megnézem", + wrong: "Hibás jelszó.", + download: "Letöltés", + file: "Fájl", + links: "Linkek", + brand: "Megosztva a Siftlode-dal", + repeat: "Ismétlés", + failed: "Nem sikerült betölteni a linket.", + failedHint: "Ellenőrizd a kapcsolatot, és próbáld újra.", + retry: "Újrapróbálás", + tooMany: "Túl sok próbálkozás. Próbáld újra pár perc múlva.", + unlockFailed: "Nem sikerült ellenőrizni a jelszót. Próbáld újra.", + crashTitle: "Valami hiba történt.", + crashHint: "Töltsd újra az oldalt, és próbáld újra.", + reload: "Újratöltés", + }, + de: { + loading: "Wird geladen…", + gone: "Dieser Link ist nicht mehr verfügbar.", + goneHint: "Er ist möglicherweise abgelaufen oder wurde widerrufen.", + passwordTitle: "Dieses Video ist passwortgeschützt", + passwordPlaceholder: "Passwort eingeben", + watch: "Ansehen", + wrong: "Falsches Passwort.", + download: "Herunterladen", + file: "Datei", + links: "Links", + brand: "Geteilt über Siftlode", + repeat: "Wiederholen", + failed: "Der Link konnte nicht geladen werden.", + failedHint: "Prüfe deine Verbindung und versuche es erneut.", + retry: "Erneut versuchen", + tooMany: "Zu viele Versuche. Versuche es in ein paar Minuten erneut.", + unlockFailed: "Das Passwort konnte nicht geprüft werden. Versuche es erneut.", + crashTitle: "Etwas ist schiefgelaufen.", + crashHint: "Lade die Seite neu und versuche es erneut.", + reload: "Neu laden", + }, +} as const; + +export const lang = + ((navigator.language || "en").slice(0, 2) as keyof typeof STR) in STR + ? ((navigator.language || "en").slice(0, 2) as keyof typeof STR) + : "en"; +export const T = STR[lang]; diff --git a/frontend/src/lib/format.test.ts b/frontend/src/lib/format.test.ts index e3773ece..096bb752 100644 --- a/frontend/src/lib/format.test.ts +++ b/frontend/src/lib/format.test.ts @@ -1,4 +1,5 @@ import { afterEach, beforeEach, describe, expect, it, vi } from "vitest"; +import type { TFunction } from "i18next"; import { RELATIVE_UNITS, formatBytes, @@ -18,12 +19,10 @@ import { import en from "../i18n/locales/en/time.json"; import hu from "../i18n/locales/hu/time.json"; -// i18n boots the whole locale glob (and touches localStorage/document) on import, so stub it. -// `language` is mutable because relativeFromMs reads it per call to pick the Intl locale — these +// `language` is mutable because relativeFromMs takes it per call to pick the Intl locale — these // cases assert both languages by flipping it. `t` echoes the key: the only string still coming // from the locale files is justNow. -const i18nStub = vi.hoisted(() => ({ language: "en", t: (key: string) => key })); -vi.mock("../i18n", () => ({ default: i18nStub })); +const i18nStub = { language: "en", t: ((key: string) => key) as unknown as TFunction }; const MIN = 60; const HOUR = 3600; @@ -74,29 +73,31 @@ describe("relativeFromMs", () => { for (const [secs, tier, expectedEn, expectedHu] of CASES) { it(`${secs}s is ${tier} — "${expectedEn}" / "${expectedHu}"`, () => { - expect(relativeFromMs(NOW - secs * 1000, i18nStub.language)).toBe(expectedEn); + expect(relativeFromMs(NOW - secs * 1000, i18nStub.language, i18nStub.t)).toBe(expectedEn); i18nStub.language = "hu"; - expect(relativeFromMs(NOW - secs * 1000, i18nStub.language)).toBe(expectedHu); + expect(relativeFromMs(NOW - secs * 1000, i18nStub.language, i18nStub.t)).toBe(expectedHu); }); } it("follows a language switch at runtime", () => { const twoHoursAgo = NOW - 2 * HOUR * 1000; - expect(relativeFromMs(twoHoursAgo, i18nStub.language)).toBe("2h ago"); + expect(relativeFromMs(twoHoursAgo, i18nStub.language, i18nStub.t)).toBe("2h ago"); i18nStub.language = "hu"; - expect(relativeFromMs(twoHoursAgo, i18nStub.language)).toBe("2 órája"); + expect(relativeFromMs(twoHoursAgo, i18nStub.language, i18nStub.t)).toBe("2 órája"); i18nStub.language = "en"; - expect(relativeFromMs(twoHoursAgo, i18nStub.language)).toBe("2h ago"); + expect(relativeFromMs(twoHoursAgo, i18nStub.language, i18nStub.t)).toBe("2h ago"); }); it("clamps a future timestamp instead of counting up", () => { - expect(relativeFromMs(NOW + 5 * DAY * 1000, i18nStub.language)).toBe("time.justNow"); + expect(relativeFromMs(NOW + 5 * DAY * 1000, i18nStub.language, i18nStub.t)).toBe( + "time.justNow", + ); }); it("renders nothing for a timestamp it cannot read", () => { - expect(relativeTime("not-a-date", i18nStub.language)).toBe(""); - expect(relativeTime(null, i18nStub.language)).toBe(""); - expect(relativeFromMs(NaN, i18nStub.language)).toBe(""); + expect(relativeTime("not-a-date", i18nStub.language, i18nStub.t)).toBe(""); + expect(relativeTime(null, i18nStub.language, i18nStub.t)).toBe(""); + expect(relativeFromMs(NaN, i18nStub.language, i18nStub.t)).toBe(""); }); // Derived from RELATIVE_UNITS, not from CASES, so adding a tier to the table without covering diff --git a/frontend/src/lib/format.ts b/frontend/src/lib/format.ts index af813387..11669904 100644 --- a/frontend/src/lib/format.ts +++ b/frontend/src/lib/format.ts @@ -1,5 +1,4 @@ import type { TFunction } from "i18next"; -import i18n from "../i18n"; // The one home of the app's LOCALE forms — numbers, units, dates (alphabetical order: `collate.ts`) — // all in the UI language, never the browser's (lint: `local/locale-format` refuses a hand-built one elsewhere). @@ -8,7 +7,9 @@ import i18n from "../i18n"; // the React Compiler memoizes a call on its arguments, so a formatter that read the language // off the i18n singleton kept its old answer after a language switch — a card still said "2 hr 4 min" // beside a hero saying "2 ó 16 p". Passing `i18n.language` (from useTranslation) makes the language a -// dependency the compiler can see; the signature makes forgetting it a type error. +// dependency the compiler can see; the signature makes forgetting it a type error. It is also why +// this file never imports the app's i18n: the public /watch entry formats a file size through it, and +// that import alone carried every locale of every area into the page a shared link opens. // Intl formatters are expensive to build and every grid formats one per card: one per language × // options, built lazily. Keyed by language because the user can switch it at runtime. @@ -23,9 +24,9 @@ function numberFormat(lang: string, opts: Intl.NumberFormatOptions = {}): Intl.N return f; } -export function relativeTime(iso: string | null, lang: string): string { +export function relativeTime(iso: string | null, lang: string, t: TFunction): string { if (!iso) return ""; - return relativeFromMs(new Date(iso).getTime(), lang); + return relativeFromMs(new Date(iso).getTime(), lang, t); } /** Human file size, binary units (1024), the number in the UI language: 501747 -> "490 KB", @@ -90,11 +91,11 @@ function relativeFormatter(lang: string): Intl.RelativeTimeFormat { * client-side notifications whose timestamps are already numbers). One implementation shared * instead of re-implemented per component. Empty string for a timestamp that can't be read, so a * caller's `|| "—"` fallback covers it like a missing one. */ -export function relativeFromMs(ms: number, lang: string): string { +export function relativeFromMs(ms: number, lang: string, t: TFunction): string { const secs = Math.max(0, (Date.now() - ms) / 1000); if (!Number.isFinite(secs)) return ""; // Sub-minute is the one case Intl has no wording for — it would say "0m ago". - if (secs < RELATIVE_UNITS[0][0]) return i18n.t("time.justNow", { lng: lang }); + if (secs < RELATIVE_UNITS[0][0]) return t("time.justNow", { lng: lang }); let [div, unit] = RELATIVE_UNITS[0]; for (const [unitSecs, unitName] of RELATIVE_UNITS) { if (secs < unitSecs) break; diff --git a/frontend/src/main.tsx b/frontend/src/main.tsx index 61d41044..d5576a18 100644 --- a/frontend/src/main.tsx +++ b/frontend/src/main.tsx @@ -29,15 +29,15 @@ installClientLog(appCopyOf); // A tab from before the last release reloads onto the new build instead of crashing on a lazy chunk. installStaleBuildReload(); -// Split by top-level route so each entry point is its own chunk: a public /watch share link or a -// legal page never downloads the authenticated app bundle, and the app never carries them either. -// The legal pages live outside the App tree (no /api/me) so Google's consent screen and reviewers -// can fetch them directly; the watch page is a login-free public player. Reached by full-page -// navigation (plain ), so a simple pathname switch is enough — no router needed. +// Split by top-level route so each entry point is its own chunk: a legal page never downloads the +// authenticated app bundle, and the app never carries it either. The legal pages live outside the +// App tree (no /api/me) so Google's consent screen and reviewers can fetch them directly. Reached by +// full-page navigation (plain ), so a simple pathname switch is enough — no router needed. +// A public /watch link is not here at all: it has its own HTML entry (watch.html → watchMain.tsx), +// so the friend it was sent to never loads this file's providers, i18n or CSS imports. const App = lazyWithPreload(() => import("./App")); const PrivacyPolicy = lazy(() => import("./components/legal/PrivacyPolicy")); const Terms = lazy(() => import("./components/legal/Terms")); -const WatchPage = lazy(() => import("./components/WatchPage")); // The design lab: the living style guide, rendered with the app's real CSS so a design decision // is taken on what will ship. DEV only — Vite folds the condition to `false` in a production build, // the import becomes dead code and the chunk is never emitted (devOnlyGuard fails the build if it is). @@ -46,7 +46,7 @@ const DesignLab = import.meta.env.DEV ? lazy(() => import("./design-lab/DesignLa const queryClient = createQueryClient(); // Baseline theme so the token CSS vars (--bg/--surface/--accent/…) resolve on first paint — including -// the standalone public pages (/watch, /privacy, /terms) that render outside and never call +// the standalone public pages (/privacy, /terms) that render outside and never call // applyTheme. The app overrides this with the user's saved theme once it mounts. const de = document.documentElement.dataset; de.theme ||= "dark"; @@ -54,8 +54,7 @@ de.scheme ||= "midnight"; const path = window.location.pathname; const isLab = DesignLab !== null && path === "/design-lab"; -const isPublicPage = - path === "/privacy" || path === "/terms" || path.startsWith("/watch/") || isLab; +const isPublicPage = path === "/privacy" || path === "/terms" || isLab; // Fetch the app chunk NOW, alongside BootGate's identity probe, instead of only once the gate opens: // the two waits then overlap rather than queue, and the gate usually opens straight onto the app. A // failed fetch is left to the render's own load, which retries it and reports to the ErrorBoundary. @@ -69,11 +68,6 @@ const root = ) : path === "/terms" ? ( - ) : path.startsWith("/watch/") ? ( - // FloatingTree so the public player's own popovers dispatch Escape/outside-press topmost-only. - - - ) : ( {/* One tree for the whole app: every anchored popover registers as a node so Escape and @@ -119,8 +113,8 @@ const root = createRoot(document.getElementById("root")!).render( - {/* Around EVERY entry, the public ones too: a crash on a shared /watch link, or its chunk gone - twice over after a release (lib/staleBuild), left a blank white page with no way on. */} + {/* Around EVERY entry, the public ones too: a crash, or a chunk gone twice over after a release + (lib/staleBuild), left a blank white page with no way on. */} }>{root} diff --git a/frontend/src/watchEntry.contract.test.ts b/frontend/src/watchEntry.contract.test.ts new file mode 100644 index 00000000..e03c935c --- /dev/null +++ b/frontend/src/watchEntry.contract.test.ts @@ -0,0 +1,109 @@ +import { describe, expect, it } from "vitest"; +import { existsSync, readFileSync, statSync } from "node:fs"; +import { dirname, join, relative, resolve } from "node:path"; +import ts from "typescript"; + +// A shared /watch link has its own entry (watch.html → watchMain.tsx) so the friend it was sent to +// loads the player page and nothing of the app. That holds only while nothing the page imports +// reaches back into the app — and the first build of the entry showed how easily it does: one +// file-size formatter imported the app's i18n, and with it every locale of every area (≈ 120 KB gzip, +// half the page). This walks the entry's static imports, as the bundler does, and names the path to +// any of what the app alone may load. +const SRC = resolve(__dirname); +const FORBIDDEN: Record = { + "src/i18n/index.ts": + "the app's i18n (every locale) — the page has its own strings (watchStrings)", + i18next: "the app's i18n runtime", + "react-i18next": "the app's i18n runtime", + "@tanstack/react-query": "the app's data layer", + "src/App.tsx": "the app shell", + "src/main.tsx": "the app's entry", + "src/lib/notifications.ts": "the app's notification center", + "src/lib/logHooks.ts": "the app's client-log wiring", + "flag-icons": "the app's flag sprites", +}; + +function resolveSpec(from: string, spec: string): string | null { + if (!spec.startsWith(".")) { + const parts = spec.split("/"); + return parts.slice(0, spec.startsWith("@") ? 2 : 1).join("/"); + } + const base = resolve(dirname(from), spec); + for (const ext of ["", ".ts", ".tsx", "/index.ts", "/index.tsx"]) { + const p = base + ext; + if (existsSync(p) && statSync(p).isFile()) return p; + } + return null; +} + +/** Specifiers a file loads at run time: value imports and re-exports, never `import type`. */ +function runtimeImports(file: string): string[] { + const sf = ts.createSourceFile(file, readFileSync(file, "utf8"), ts.ScriptTarget.Latest, true); + const specs: string[] = []; + for (const st of sf.statements) { + if ( + ts.isImportDeclaration(st) && + st.importClause?.phaseModifier !== ts.SyntaxKind.TypeKeyword + ) { + specs.push((st.moduleSpecifier as ts.StringLiteral).text); + } else if (ts.isExportDeclaration(st) && st.moduleSpecifier && !st.isTypeOnly) { + specs.push((st.moduleSpecifier as ts.StringLiteral).text); + } + } + return specs; +} + +const key = (p: string) => + p.startsWith("/") || /^[A-Za-z]:/.test(p) + ? relative(join(SRC, ".."), p).split("\\").join("/") + : p; + +/** Every module the entry reaches, each with the importer that first reached it. */ +function reach(entry: string): Map { + const seen = new Map([[key(entry), null]]); + const queue = [entry]; + for (let file = queue.shift(); file; file = queue.shift()) { + if (!/\.tsx?$/.test(file)) continue; + for (const spec of runtimeImports(file)) { + const target = resolveSpec(file, spec); + if (target === null || seen.has(key(target))) continue; + seen.set(key(target), key(file)); + if (!target.startsWith("@") && /\.tsx?$/.test(target)) queue.push(target); + } + } + return seen; +} + +function chain(seen: Map, mod: string): string { + const path = [mod]; + for (let up = seen.get(mod); up; up = seen.get(up)) path.unshift(up); + return path.join(" → "); +} + +describe("the /watch entry", () => { + it("loads nothing the app alone may load", () => { + const seen = reach(join(SRC, "watchMain.tsx")); + expect(seen.has("src/components/WatchPage.tsx")).toBe(true); // the walk is reading the entry + const leaks = Object.keys(FORBIDDEN) + .filter((mod) => seen.has(mod)) + .map((mod) => `${FORBIDDEN[mod]}: ${chain(seen, mod)}`); + expect(leaks).toEqual([]); + }); + + it("catches a leak one import deep (the walk is not vacuous)", () => { + // main.tsx is the app: walking it must find the app's i18n. + expect(reach(join(SRC, "main.tsx")).has("src/i18n/index.ts")).toBe(true); + }); + + it("serves the same head as the app's page, so the server's OG block lands the same way", () => { + const page = (name: string) => readFileSync(join(SRC, "..", name), "utf8"); + // Comments aside (the OG markers are kept: the server replaces between them), and the script. + const norm = (html: string) => + html + .replace(//g, "") + .replace(/\s+/g, " ") + .replace(/src="\/src\/[^"]+"/, 'src="ENTRY"'); + expect(norm(page("watch.html"))).toBe(norm(page("index.html"))); + expect(page("watch.html")).toContain('src="/src/watchMain.tsx"'); + }); +}); diff --git a/frontend/src/watchMain.tsx b/frontend/src/watchMain.tsx new file mode 100644 index 00000000..6302f0e7 --- /dev/null +++ b/frontend/src/watchMain.tsx @@ -0,0 +1,26 @@ +import React from "react"; +import { createRoot } from "react-dom/client"; +import { CrashBoundary } from "./components/CrashBoundary"; +import WatchPage from "./components/WatchPage"; +import { T } from "./components/watchStrings"; +import "./index.css"; +import { installStaleBuildReload } from "./lib/staleBuild"; + +// The entry of a public /watch/ link (watch.html). Whoever opens it got the link from a +// friend: signed out, often on a phone, often on mobile data — so it imports the player page and +// what that page renders, and none of main.tsx's app (no providers, no app i18n, no client-log +// wiring: an anonymous viewer has no diagnostics consent to send it under). A contract test walks +// this file's imports and fails on any of those (`watchEntry.contract.test.ts`). + +// A tab left open across a release reloads onto the new build instead of failing on a chunk. +installStaleBuildReload(); + +const crashCopy = () => ({ title: T.crashTitle, subtitle: T.crashHint, reload: T.reload }); + +createRoot(document.getElementById("root")!).render( + + + + + , +); diff --git a/frontend/vite.config.ts b/frontend/vite.config.ts index 51197592..cde5ed87 100644 --- a/frontend/vite.config.ts +++ b/frontend/vite.config.ts @@ -1,4 +1,4 @@ -import { defineConfig } from "vite"; +import { defineConfig, type Plugin } from "vite"; import { reactWithCompiler } from "./reactPlugin"; import { devOnlyGuard } from "./devOnlyGuard"; @@ -17,8 +17,21 @@ const proxy = { "/healthz": proxyOpts, }; +// Two HTML entries: the app (index.html) and a public /watch/ link (watch.html), which loads +// only the player page. In a build the backend picks the file by path (`main.py`); this does the +// same for `vite dev`, whose history fallback would otherwise hand /watch/… the app. +const watchEntry = (): Plugin => ({ + name: "siftlode-watch-entry", + configureServer(server) { + server.middlewares.use((req, _res, next) => { + if (req.url?.startsWith("/watch/")) req.url = "/watch.html"; + next(); + }); + }, +}); + export default defineConfig({ - plugins: [reactWithCompiler(), devOnlyGuard()], + plugins: [reactWithCompiler(), devOnlyGuard(), watchEntry()], server: { port: 5173, proxy }, - build: { outDir: "dist" }, + build: { outDir: "dist", rollupOptions: { input: ["index.html", "watch.html"] } }, }); diff --git a/frontend/watch.html b/frontend/watch.html new file mode 100644 index 00000000..97796a77 --- /dev/null +++ b/frontend/watch.html @@ -0,0 +1,35 @@ + + + + + + + + + + Siftlode + + + + + + + +
+ + + From e7d2eb8cf6f3cd25f1f316ee542f809800621eca Mon Sep 17 00:00:00 2001 From: npeter83 Date: Mon, 5 Oct 2026 21:05:22 +0200 Subject: [PATCH 04/12] test(downloads): what a share link serves passes the phone-safe rule MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit A link serves an asset once it is ready, so the contract is pinned on both sides. The code: every function in app/ that marks something ready must run the rule (`_ensure_phone_safe`, or the upload's own `playable.plan`) — an AST scan names any that does not, and fails blind if it stops finding the four paths known today (download, editor clip, upload conversion, upload kept as is). The bytes: the files an iPhone + iPad refused (HEVC tagged hev1, AV1, Matroska, MP3 inside an .mp4) and the ones other browsers cannot be trusted with (VP9 + Opus, 10-bit H.264, Opus in Ogg) go through the upload route, the worker's conversion and the public file route; what the link hands out must be served as mp4 / m4a / mp3 and pass the rule. Seen red: a planted ready-marking function in service.py is named; with the conversion step short-circuited, all seven files fail (on the rule, or on being served as quicktime / webm / ogg). --- backend/tests/test_phone_safe_contract.py | 163 ++++++++++++++++++++++ 1 file changed, 163 insertions(+) create mode 100644 backend/tests/test_phone_safe_contract.py diff --git a/backend/tests/test_phone_safe_contract.py b/backend/tests/test_phone_safe_contract.py new file mode 100644 index 00000000..50505f73 --- /dev/null +++ b/backend/tests/test_phone_safe_contract.py @@ -0,0 +1,163 @@ +"""The share-link contract: every media file a /watch link can serve passes the phone-safe rule +(`downloads.playable`). + +Two halves. The structural one reads the code: a link serves an asset once it is `ready`, so every +function that marks one ready must run the rule — a new path to `ready` that skips it is named here, +before a friend's phone meets the file. The end-to-end one sends the files measured broken on an +iPhone + iPad (and the ones browsers other than Safari cannot be trusted with) through upload, the +worker's conversion and the public file route, and judges the bytes the link actually hands out. +""" + +import ast +import shutil +import subprocess +from pathlib import Path + +import pytest + +from app.config import settings +from app.downloads import pipeline, playable, probe +from app.models import DownloadJob, MediaAsset + +APP = Path(__file__).resolve().parents[1] / "app" + +# The calls that ARE the rule: the worker's step (download, clip, upload conversion) and the upload's +# judgement of a file it keeps as it came. +_RULE_CALLS = {"_ensure_phone_safe", "plan"} + + +def _marks_ready(node: ast.AST) -> bool: + """`x.status = "ready"`, `status="ready"`, or `"…" if … else "ready"` handed to either.""" + + def is_ready(v: ast.AST | None) -> bool: + if isinstance(v, ast.Constant): + return v.value == "ready" + if isinstance(v, ast.IfExp): + return is_ready(v.body) or is_ready(v.orelse) + return False + + if isinstance(node, ast.Assign): + return is_ready(node.value) and any( + isinstance(t, ast.Attribute) and t.attr == "status" for t in node.targets + ) + return isinstance(node, ast.keyword) and node.arg == "status" and is_ready(node.value) + + +def _calls(fn: ast.AST) -> set[str]: + names = set() + for n in ast.walk(fn): + if isinstance(n, ast.Call): + f = n.func + names.add(f.attr if isinstance(f, ast.Attribute) else getattr(f, "id", "")) + return names + + +def ready_sites(source: str) -> list[tuple[str, int, bool]]: + """(function, line, runs the rule) for every place `source` marks something ready.""" + sites = [] + for fn in ast.walk(ast.parse(source)): + if not isinstance(fn, ast.FunctionDef | ast.AsyncFunctionDef): + continue + runs_rule = bool(_calls(fn) & _RULE_CALLS) + for n in ast.walk(fn): + if _marks_ready(n): + sites.append((fn.name, n.lineno, runs_rule)) + return sites + + +def test_every_path_to_ready_runs_the_rule(): + found, skipped = [], [] + for path in sorted(APP.rglob("*.py")): + for name, line, runs_rule in ready_sites(path.read_text(encoding="utf-8")): + where = f"{path.relative_to(APP.parent).as_posix()}:{line} {name}" + found.append(where) + if not runs_rule: + skipped.append(where) + # The paths known today: a download, an editor clip, an upload's conversion, an upload kept as is. + # Fewer means the scan went blind (a rename, a new status spelling) — not that a path is gone. + names = {w.rsplit(" ", 1)[1] for w in found} + assert {"_download", "_process_edit", "_convert_upload", "ingest_upload"} <= names, found + assert skipped == [], f"marks an asset ready without the phone-safe rule: {skipped}" + + +def test_the_scan_names_a_path_that_skips_the_rule(): + planted = 'def restore(asset):\n asset.status = "ready"\n' + guarded = ( + 'def restore(asset, p):\n _ensure_phone_safe(1, p, p, "x")\n asset.status = "ready"\n' + ) + assert ready_sites(planted) == [("restore", 2, False)] + assert ready_sites(guarded) == [("restore", 3, True)] + assert ( + ready_sites('def f(a, x):\n a.update(status="pending" if x else "ready")\n')[0][2] + is False + ) + + +# --- end to end: upload → worker → the bytes a link serves --------------------------------------- + +_V = ["-f", "lavfi", "-i", "testsrc2=size=160x120:rate=10:duration=1"] +_A = ["-f", "lavfi", "-i", "sine=duration=1"] + +# name -> (ffmpeg args after the inputs, filename the user uploads) +_FILES = { + "hevc tagged hev1 (iOS: broken)": ( + [*_V, *_A, "-c:v", "libx265", "-tag:v", "hev1", "-c:a", "aac"], + "phone.mov", + ), + "av1 (iOS: broken)": ([*_V, *_A, "-c:v", "libsvtav1", "-c:a", "aac"], "clip.mp4"), + "h264 in matroska (iOS: broken)": ([*_V, *_A, "-c:v", "libx264", "-c:a", "aac"], "clip.mkv"), + "mp3 inside an mp4 (iOS: broken)": ([*_A, "-c:a", "libmp3lame", "-f", "mp4"], "song.mp4"), + "vp9 + opus webm": ([*_V, *_A, "-c:v", "libvpx-vp9", "-c:a", "libopus"], "clip.webm"), + "10-bit h264": ( + [*_V, *_A, "-c:v", "libx264", "-pix_fmt", "yuv420p10le", "-c:a", "aac"], + "clip.mp4", + ), + "opus in ogg": ([*_A, "-c:a", "libopus"], "voice.ogg"), +} + + +@pytest.fixture +def media_root(tmp_path, monkeypatch): + root = tmp_path / "media" + root.mkdir() + monkeypatch.setattr(settings, "download_root", str(root)) + return root + + +@pytest.mark.skipif(shutil.which("ffmpeg") is None, reason="ffmpeg not available") +@pytest.mark.parametrize("name", _FILES) +def test_what_a_link_serves_passes_the_rule( + name, db, as_user, make_user, client, media_root, tmp_path, upload_headers +): + args, filename = _FILES[name] + src = tmp_path / f"src{Path(filename).suffix}" + subprocess.run(["ffmpeg", "-y", "-loglevel", "error", *args, str(src)], check=True) + + owner = as_user(make_user()) + r = owner.post( + "/api/downloads/upload", content=src.read_bytes(), headers=upload_headers(filename) + ) + assert r.status_code == 200, r.text + job_id = r.json()["id"] + assert r.json()["status"] == "queued" # every one of these fails the rule as it came + job = db.get(DownloadJob, job_id) + asset = db.get(MediaAsset, job.asset_id) + job.status, asset.status = "running", "downloading" # what the worker's claim does + db.commit() + pipeline._convert_upload(job.id, asset.id) + db.expire_all() + assert db.get(DownloadJob, job_id).status == "done" + + link = owner.post(f"/api/downloads/{job_id}/links", json={}) + assert link.status_code == 200, link.text + served = client.get(f"/api/public/watch/{link.json()['token']}/file") + assert served.status_code == 200 + got = tmp_path / "served.bin" + got.write_bytes(served.content) + data = probe.probe_json(got) + ext = {"video/mp4": "mp4", "audio/mp4": "m4a", "audio/mpeg": "mp3"}.get( + served.headers["content-type"].split(";")[0] + ) + assert ext is not None, served.headers["content-type"] # served as a type every phone plays + verdict = playable.plan(data, ext) + assert verdict is not None and not verdict.needs_change, (name, data["streams"]) From 00886d92febb3166de2215f65fa202ceedbc6ba8 Mon Sep 17 00:00:00 2001 From: npeter83 Date: Mon, 5 Oct 2026 23:40:13 +0200 Subject: [PATCH 05/12] fix(player): the YouTube player keeps its size on a short window and when maximised MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Two layout bugs since the player grew a side panel, both reported from prod. Maximised (F) on a wide window, the video was a thin line across the screen: the side-panel layout aligns the stage `self-start`, and Chrome applies a flex item's self-alignment to an absolutely positioned box too, so the fixed stage took its content's height — 0 px. The maximised stage now stretches between its insets whatever the layout asks of it. On a short window under 1280 px (a laptop at 100%, a 1080p screen at 175%) only the side strips were left: the card narrows so the chrome under the video fits, but that chrome grows as the card narrows (its text wraps), and the loop ran the card down to 2 px. The video now never goes narrower than 36rem to make room; past that the card scrolls. Measured before/after (localdev, real YouTube iframe): 1097×520 open 0×0 → 610×343; 1920×911 maximised 1920×0 → 1920×911; 1440×760 and 1366×650 maximised fill the window; leaving maximise restores the card. Held by an e2e test at both sizes (red on the old code: 0 px wide, and 911 ≠ 0), green on Chrome, WebKit and iPad. --- frontend/e2e/player.spec.ts | 40 ++++++++++++++++++- .../src/components/PlayerModal.dom.test.tsx | 6 ++- frontend/src/components/PlayerModal.tsx | 15 +++++-- frontend/src/index.css | 6 +++ 4 files changed, 60 insertions(+), 7 deletions(-) diff --git a/frontend/e2e/player.spec.ts b/frontend/e2e/player.spec.ts index 0fdc3bfd..8a234fa9 100644 --- a/frontend/e2e/player.spec.ts +++ b/frontend/e2e/player.spec.ts @@ -1,4 +1,4 @@ -import { test, expect } from "@playwright/test"; +import { test, expect, type Page } from "@playwright/test"; import { openApp, openFirstVideo } from "./helpers"; // Locks the in-app player's open/close contract. The player is a portaled overlay whose @@ -26,3 +26,41 @@ test.describe("player modal", () => { await expect(page.getByTestId("player-modal")).toHaveCount(0); }); }); + +// The video's size, in the windows the owner meets: layout bugs no DOM test can see. Each viewport +// is a real one — 1920×911 is a 1080p screen at 100% (the side-panel layout), 1097×520 the same +// screen at 175% or a small laptop at 100% (the stacked layout, on a short window). +test.describe("player modal sizing", () => { + test.beforeEach(async ({ page }) => { + await page.route(/youtube\.com/, (route) => route.abort()); + }); + + const box = async (page: Page, selector: string) => + (await page.locator(selector).first().boundingBox())!; + + for (const [w, h] of [ + [1920, 911], + [1097, 520], + ] as const) { + test(`the video keeps a real size at ${w}×${h}, and maximised fills the window`, async ({ + page, + }) => { + await page.setViewportSize({ width: w, height: h }); + await openApp(page); + await openFirstVideo(page); + + // A short window narrows the card so the chrome under the video fits — but the chrome grows + // as the card narrows (its text wraps), and that loop once ran the card down to 2 px. + const stage = await box(page, ".player-stage"); + expect(stage.width).toBeGreaterThanOrEqual(Math.min(w * 0.5, 560)); + expect(stage.height).toBeGreaterThan(stage.width * 0.5); + + // Maximise (F): the stage is fixed to the viewport. In the side-panel layout its `self-start` + // once aligned the fixed box too, and it shrank to its content's height — 0 px, a thin line. + await page.getByTestId("player-modal").press("f"); + await expect(page.locator(".player-stage--max")).toHaveCount(1); + const max = await box(page, ".player-stage--max"); + expect([Math.round(max.width), Math.round(max.height)]).toEqual([w, h]); + }); + } +}); diff --git a/frontend/src/components/PlayerModal.dom.test.tsx b/frontend/src/components/PlayerModal.dom.test.tsx index f856b5cd..21d3a03d 100644 --- a/frontend/src/components/PlayerModal.dom.test.tsx +++ b/frontend/src/components/PlayerModal.dom.test.tsx @@ -465,7 +465,9 @@ describe("PlayerModal — fits the window's height", () => { {}} onState={() => {}} />, ); const row = document.querySelector(".player-stage")!.parentElement!.parentElement!; - expect(row.style.maxWidth).toBe("min(63rem, calc((100dvh - 3rem - 160px) * 16 / 9 + 7rem))"); + expect(row.style.maxWidth).toBe( + "min(63rem, calc(max(36rem, (100dvh - 3rem - 160px) * 16 / 9) + 7rem))", + ); }); }); @@ -510,7 +512,7 @@ describe("PlayerModal — the side panel on a wide window", () => { expect(screen.queryByRole("button", { name: "player.description" })).toBeNull(); // The row's width: the video at the window's height, plus the panel and the side strips. expect(card.parentElement!.style.maxWidth).toBe( - "calc((100dvh - 3rem - 0px) * 16 / 9 + 20rem + 7rem)", + "calc(max(36rem, (100dvh - 3rem - 0px) * 16 / 9) + 20rem + 7rem)", ); }); diff --git a/frontend/src/components/PlayerModal.tsx b/frontend/src/components/PlayerModal.tsx index e6506986..bce2d2b1 100644 --- a/frontend/src/components/PlayerModal.tsx +++ b/frontend/src/components/PlayerModal.tsx @@ -104,6 +104,8 @@ function canScrollBy(el: HTMLElement | null, dir: -1 | 1): boolean { // a rem width, so it grows with the text size; the video takes what is left. const WIDE_PLAYER_PX = 1280; const SIDE_PANEL = "20rem"; +// The narrowest the video gets to make room for the chrome under it (see the card's maxWidth). +const MIN_VIDEO = "36rem"; // The meta line's items each lead with a "·" that parts them from what stands before: the channel name // on its line. In the wide panel the meta has a line of its own, where the first "·" would lead nothing. @@ -611,6 +613,10 @@ export default function PlayerModal({ onLoadVideo: loadVideo, }; + // The card's width budget (its maxWidth, below): the video's, and the side strips' gutters. + const videoWidth = `max(${MIN_VIDEO}, (100dvh - 3rem - ${below}px) * 16 / 9)`; + const gutters = hasQueue ? " + 7rem" : ""; + // Portaled to , like every other full-screen overlay here. A fixed, full-viewport modal // must not render inside the page scroller: once that scroller is a stacking context (its // edge-fade mask makes it one) this z-overlay is scoped inside it, and the nav rail (z-rail) and the @@ -636,12 +642,13 @@ export default function PlayerModal({ // The design width (63rem with the side strips, else 56rem), narrowed so the 16:9 video plus the // chrome under it fits the window's height less the backdrop's padding (`p-6` = 3rem). // Wide: no design cap — the video is as wide as the window's height allows, plus the side panel. + // Never narrower than MIN_VIDEO: the chrome under the video grows as the card narrows (its text + // wraps), so on a short window fitting it was a loop that ran the card down to 2 px (a laptop + // at 100%, a 1080p screen at 175%). Past the floor the card scrolls instead. style={{ maxWidth: wide - ? `calc((100dvh - 3rem - ${below}px) * 16 / 9 + ${SIDE_PANEL}${hasQueue ? " + 7rem" : ""})` - : `min(${hasQueue ? "63rem" : "56rem"}, calc((100dvh - 3rem - ${below}px) * 16 / 9${ - hasQueue ? " + 7rem" : "" - }))`, + ? `calc(${videoWidth} + ${SIDE_PANEL}${gutters})` + : `min(${hasQueue ? "63rem" : "56rem"}, calc(${videoWidth}${gutters}))`, }} > {hasQueue && ( diff --git a/frontend/src/index.css b/frontend/src/index.css index 686669d7..d6adc705 100644 --- a/frontend/src/index.css +++ b/frontend/src/index.css @@ -1176,6 +1176,12 @@ html[data-theme="light"] { .player-stage--max { position: fixed; inset: 0; + /* Stretched between the insets, whatever the layout asks of the in-flow stage: the side-panel + layout aligns it `self-start`, and Chrome applies a flex item's self-alignment to an absolutely + positioned box too — the fixed stage then took its content's height, 0 px (its content is all + absolutely positioned): a thin line across the window instead of the video. */ + align-self: stretch; + justify-self: stretch; /* rail level: this stage only has to beat the modal chrome it is nested inside (whose own children top out at the menu level). The modal root is a z-overlay stacking context, so nothing here escapes it — a higher level would claim an authority this layer neither has nor needs. */ From 68cfc7e25dd3eeee0130562f5bd48b69042515a2 Mon Sep 17 00:00:00 2001 From: npeter83 Date: Tue, 6 Oct 2026 03:19:32 +0200 Subject: [PATCH 06/12] fix(downloads): an upload's conversion is guarded, counted, named and cleaned up MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit - Every ffmpeg/ffprobe over a library file names the demuxers it may use (edit.INPUT_GUARD; a downloaded thumbnail edit.THUMB_GUARD): a playlist-type demuxer would read local paths named inside a user's upload. - A re-encode ends on an even frame (edit.EVEN_FRAME): an odd 4:4:4/RGB source failed its conversion; a clip stores the frame its file has. - An upload counts against its owner's cap from the moment it lands (queued, or kept after a failed conversion) — the footprint, the GC's cap and the admin view read one ON_DISK predicate — and a conversion that outgrows the cap is refused, keeping the original. - The converted name keeps _a{id} (a dotted bare name once lost it and two uploads shared a file); a converted file placed but not committed is removed, and the orphan sweep enters .uploads; the LRU cap never evicts an upload. - Ingest judges the extension the file is stored under; a failed conversion is convert_failed / upload_missing, reports the "converting" phase, and notifies as an upload. Job statuses in routes/downloads use JobStatus. --- backend/app/downloads/edit.py | 62 ++- backend/app/downloads/errors.py | 12 + backend/app/downloads/gc.py | 50 +-- backend/app/downloads/pipeline.py | 62 ++- backend/app/downloads/playable.py | 6 +- backend/app/downloads/quota.py | 43 ++- backend/app/downloads/storage.py | 16 +- backend/app/downloads/upload.py | 15 +- backend/app/models.py | 3 +- backend/app/routes/downloads.py | 26 +- backend/tests/test_ffmpeg_input_guard.py | 163 ++++++++ backend/tests/test_odd_frame.py | 73 ++++ backend/tests/test_upload_conversion.py | 355 ++++++++++++++++++ backend/tests/test_worker_dispatch.py | 74 ++++ .../src/components/{ => ui}/CrashBoundary.tsx | 0 15 files changed, 892 insertions(+), 68 deletions(-) create mode 100644 backend/tests/test_ffmpeg_input_guard.py create mode 100644 backend/tests/test_odd_frame.py create mode 100644 backend/tests/test_upload_conversion.py create mode 100644 backend/tests/test_worker_dispatch.py rename frontend/src/components/{ => ui}/CrashBoundary.tsx (100%) diff --git a/backend/app/downloads/edit.py b/backend/app/downloads/edit.py index 38c90f5d..f340ff04 100644 --- a/backend/app/downloads/edit.py +++ b/backend/app/downloads/edit.py @@ -194,6 +194,44 @@ def clip_duration(spec: dict, source_duration: int | None) -> int | None: # only patchily (the one safe H.264 is 8-bit 4:2:0). X264_ARGS = ["-c:v", "libx264", "-preset", "veryfast", "-crf", "20", "-pix_fmt", "yuv420p"] AAC_ARGS = ["-c:a", "aac", "-b:a", "192k"] +# The last step of every video filter chain a libx264 re-encode runs: 4:2:0 halves the chroma, so +# libx264 refuses an odd width or height ("width not divisible by 2") — measured on an odd source +# that is not 4:2:0 already (a 4:4:4 or RGB screen capture, 1365x767). A crop of a 4:2:0 source is +# rounded to its chroma grid by ffmpeg itself; this holds every other path to the encoder to the same +# even frame. Rounding down to even costs at most one pixel row/column. +EVEN_FRAME = "scale=trunc(iw/2)*2:trunc(ih/2)*2" + +# The demuxers ffmpeg/ffprobe may use to open a library file — single-file containers only. Without +# it ffmpeg picks the demuxer from the file's CONTENT and NAME, and an upload's name is the user's: +# a playlist-type demuxer (HLS, DASH, concat) opens whatever local paths its entries name, so a +# crafted upload could copy another user's clip or a Plex file into its own converted asset. A file +# no demuxer here can open fails the probe, and an unprobeable upload is a download-only 'file'. +MEDIA_DEMUXERS = ( + "mov", # mp4, m4a, mov, 3gp + "matroska", # mkv, webm + "mpegts", + "mpeg", + "avi", + "asf", + "flv", + "ogg", + "mp3", + "aac", + "ac3", + "eac3", + "flac", + "wav", + "w64", + "aiff", + "caf", + "amr", +) +INPUT_GUARD = ["-format_whitelist", ",".join(MEDIA_DEMUXERS)] +# A downloaded thumbnail is the remote site's bytes under the suffix yt-dlp gave it (`.png`, `.webp`, +# `.jpg` — `pipeline._THUMB_SUFFIXES`): only the single-image demuxers those open, so a "thumbnail" +# that is really a playlist cannot pull a local file into the poster. Anything else fails, and the +# poster falls back to a cut frame (`ensure_poster`). +THUMB_GUARD = ["-format_whitelist", "png_pipe,jpeg_pipe,webp_pipe"] def build_edit_cmd(src: Path, dest: Path, spec: dict, out_ext: str) -> list[str]: @@ -221,7 +259,7 @@ def build_edit_cmd(src: Path, dest: Path, spec: dict, out_ext: str) -> list[str] ] if start: cmd += ["-ss", f"{start:.3f}"] - cmd += ["-i", str(src)] + cmd += [*INPUT_GUARD, "-i", str(src)] if end is not None: dur = end - start if dur > 0: @@ -229,10 +267,9 @@ def build_edit_cmd(src: Path, dest: Path, spec: dict, out_ext: str) -> list[str] if reencode: # Input -ss + re-encode is frame-accurate (decodes from the prior keyframe, discards up to - # the exact cut). A crop adds the filter; an accurate trim re-encodes with no filter. - if crop: - cmd += ["-vf", f"crop={crop['w']}:{crop['h']}:{crop['x']}:{crop['y']}"] - cmd += [*X264_ARGS, *AAC_ARGS] + # the exact cut). A crop adds the filter; an accurate trim only evens the frame. + cropf = f"crop={crop['w']}:{crop['h']}:{crop['x']}:{crop['y']}," if crop else "" + cmd += ["-vf", cropf + EVEN_FRAME, *X264_ARGS, *AAC_ARGS] else: cmd += ["-c", "copy", "-avoid_negative_ts", "make_zero"] @@ -280,11 +317,14 @@ def build_concat_plan(src: Path, dest: Path, spec: dict, out_ext: str, staging: parts, labels = [], [] for i, s in enumerate(segs): st, en = s["start_s"], s["end_s"] - parts.append(f"[0:v]trim=start={st}:end={en},setpts=PTS-STARTPTS{cropf}[v{i}]") + parts.append( + f"[0:v]trim=start={st}:end={en},setpts=PTS-STARTPTS{cropf},{EVEN_FRAME}[v{i}]" + ) parts.append(f"[0:a]atrim=start={st}:end={en},asetpts=PTS-STARTPTS[a{i}]") labels.append(f"[v{i}][a{i}]") parts.append(f"{''.join(labels)}concat=n={len(segs)}:v=1:a=1[v][a]") cmd = head + [ + *INPUT_GUARD, "-i", str(src), "-filter_complex", @@ -304,7 +344,9 @@ def build_concat_plan(src: Path, dest: Path, spec: dict, out_ext: str, staging: lines.append(f"inpoint {s['start_s']}") lines.append(f"outpoint {s['end_s']}") prep[listp] = "\n".join(lines) + "\n" - cmd = head + ["-f", "concat", "-safe", "0", "-i", str(listp), "-c", "copy"] + # The list is ours; the whitelist reaches the file it names too (ffmpeg hands it down). + guard = ["-format_whitelist", "concat," + INPUT_GUARD[1]] + cmd = head + ["-f", "concat", "-safe", "0", *guard, "-i", str(listp), "-c", "copy"] if out_ext in ("mp4", "m4a", "mov"): cmd += ["-movflags", "+faststart"] @@ -477,7 +519,7 @@ def run_ffprobe_json( package: the shared media probe (`probe.probe_json`: the stored facts and the phone-safe rule) goes through it, so the subprocess flags/timeout/error handling live in one place instead of drifting copies.""" - args = ["ffprobe", "-v", "error"] + args = ["ffprobe", "-v", "error", *INPUT_GUARD] if show_streams: args.append("-show_streams") if show_format: @@ -507,6 +549,7 @@ def probe_duration(path: Path) -> int: "ffprobe", "-v", "error", + *INPUT_GUARD, "-show_entries", "format=duration", "-of", @@ -564,6 +607,7 @@ def build_storyboard_cmd(src: Path, dest: Path, duration_s: int | None, geom: di "-loglevel", "error", "-nostdin", + *INPUT_GUARD, "-i", str(src), "-frames:v", @@ -587,6 +631,7 @@ def build_poster_cmd(src: Path, dest: Path, at_s: float) -> list[str]: "-nostdin", "-ss", f"{max(0.0, at_s):.3f}", + *INPUT_GUARD, "-i", str(src), "-frames:v", @@ -608,6 +653,7 @@ def build_thumbnail_jpg_cmd(src: Path, dest: Path) -> list[str]: "-loglevel", "error", "-nostdin", + *THUMB_GUARD, "-i", str(src), "-frames:v", diff --git a/backend/app/downloads/errors.py b/backend/app/downloads/errors.py index 3475d5f2..e390d355 100644 --- a/backend/app/downloads/errors.py +++ b/backend/app/downloads/errors.py @@ -53,6 +53,7 @@ from app.downloads.credentials import host_key_for_url # constants, imported above; each has a rule below that gives its category back, and a catalog entry # that gives the site's sentence. QUOTA_FULL_MESSAGE = "Your download storage is full — free up space, then try again." +UPLOAD_MISSING_MESSAGE = "The uploaded file is missing." NETWORK_REDACTED_MESSAGE = "The server could not reach that address." # (marker, category) — first match wins, so order most-specific → most-general. Matching is @@ -98,6 +99,8 @@ _RULES: tuple[tuple[str, str], ...] = ( # An edit's source download went away — our own wording, and MORE specific than the generic # "no longer available" below, so it must be matched first. ("source download is no longer available", "source_missing"), + # An upload's original went away before its conversion ran — our wording (UPLOAD_MISSING_MESSAGE). + (UPLOAD_MISSING_MESSAGE.lower().rstrip("."), "upload_missing"), ("source file is missing", "source_missing"), # Private / deleted / removed / terminated. ("private video", "unavailable"), @@ -263,6 +266,15 @@ def classify(msg: str | None) -> str: return "unknown" +def classify_conversion(msg: str | None) -> str: + """The category of an upload's failed conversion. The generic ffmpeg categories speak of a + download ("the video downloaded but…"), which an upload never was: a failed encode or an + unrecognised failure of one is `convert_failed`; a category of its own (a full quota, a missing + original) stays as it is.""" + code = classify(msg) + return "convert_failed" if code in ("postprocess", "unknown") else code + + def diagnose( msg: str | None, *, url: str | None = None, cookie_attached: bool = False ) -> Diagnosis: diff --git a/backend/app/downloads/gc.py b/backend/app/downloads/gc.py index 8218cf40..7a9c2afd 100644 --- a/backend/app/downloads/gc.py +++ b/backend/app/downloads/gc.py @@ -6,12 +6,13 @@ Five passes each run: 2. Expiry deletion — delete assets past their TTL (files + row). The FK SET NULL nulls the holders' jobs' asset_id, so those jobs surface as "expired" (re-downloadable) in the UI. 3. LRU eviction — if a total cache cap is set and exceeded, delete least-recently-accessed - ready assets until back under it. + ready assets (never an upload) until back under it. 4. Orphan reap — delete errored, fileless, unreferenced asset rows (they carry no TTL, so passes 1-3 skip them; benign row-count hygiene). 5. Orphan FILE sweep — delete on-disk files/dirs no surviving asset owns (cruft left when a - re-download renames an asset, e.g. old accented channel dirs), skipping system trees and - anything newer than an hour so an in-flight download is never touched. + re-download renames an asset, e.g. old accented channel dirs, or an upload's stranded file). + It skips the dot-prefixed system trees (all but `.uploads`) and anything newer than an hour, + so an in-flight download is never touched. Runs in the API process (which mounts DOWNLOAD_ROOT). Pure disk/DB work — no YouTube quota. """ @@ -20,19 +21,19 @@ import time from datetime import datetime, timedelta, timezone from pathlib import Path -from sqlalchemy import func, select +from sqlalchemy import func, or_, select from sqlalchemy.orm import Session from app import logs, sysconfig from app.config import settings -from app.downloads import storage +from app.downloads import quota, storage from app.models import DownloadJob, MediaAsset from app.notifications import create_notification log = logs.get_logger(__name__) -# Dot-prefixed top-level trees that are NOT part of the Plex/flat asset layout — the orphan sweep -# never touches them (user clips, Plex HLS/img caches, in-progress staging, editor storyboards). -_SYSTEM_TREE = {".edits", ".plex-hls", ".plex-img-cache", ".staging", ".storyboards"} +# The one dot-prefixed top-level tree the orphan sweep enters. The others are not part of the asset +# layout (user clips, Plex HLS/img caches, in-progress staging, editor storyboards) and are skipped. +_UPLOAD_TREE = ".uploads" _ORPHAN_MIN_AGE_S = 3600 # only sweep files older than this, so an in-flight download is never hit @@ -67,16 +68,20 @@ def _delete_asset(db: Session, asset: MediaAsset, ntype: str) -> None: def _sweep_orphan_files(db: Session, download_root: str) -> int: """Delete media/sidecar files (and now-empty dirs) under DOWNLOAD_ROOT that no `ready` asset owns — cruft left when a re-download changes an asset's on-disk name (e.g. an old accented - channel dir, or a poster from a superseded naming scheme). Skips the dot-prefixed system trees - and only removes files older than `_ORPHAN_MIN_AGE_S`, so a just-completed or in-flight download - is never deleted, and only files NOT owned by a surviving asset.""" + channel dir, or a poster from a superseded naming scheme) — or, under `.uploads`, that no upload + row names at all. Skips the other dot-prefixed system trees and only removes files older than + `_ORPHAN_MIN_AGE_S`, so a just-completed or in-flight download is never deleted, and only files + NOT owned by a surviving asset.""" root = Path(download_root) if not root.is_dir(): return 0 keep: set[Path] = set() + # An upload's file is on disk from ingest on, whatever its state (a queued conversion's original, + # a failed one kept for Retry), so every upload row keeps its file — not only a ready one. for rel in db.execute( select(MediaAsset.rel_path).where( - MediaAsset.status == "ready", MediaAsset.rel_path.is_not(None) + or_(MediaAsset.status == "ready", MediaAsset.source_kind == "upload"), + MediaAsset.rel_path.is_not(None), ) ).scalars(): media = (root / rel).resolve() @@ -99,7 +104,10 @@ def _sweep_orphan_files(db: Session, download_root: str) -> int: unreclaimed = 0 for entry in root.iterdir(): - if entry.name.startswith("."): # .edits / .plex-* / .staging / .storyboards + any dotfile + # .edits / .plex-* / .staging / .storyboards + any dotfile — but `.uploads` is swept: a + # conversion stopped between placing the new file and the commit that points at it (or the + # unlink of the original after it) leaves a file no row names, and nothing else finds it. + if entry.name.startswith(".") and entry.name != _UPLOAD_TREE: continue files = [entry] if entry.is_file() else [p for p in entry.rglob("*") if p.is_file()] for f in files: @@ -188,17 +196,17 @@ def run_download_gc(db: Session) -> dict: if cap > 0: total = ( db.execute( - select(func.coalesce(func.sum(MediaAsset.size_bytes), 0)).where( - MediaAsset.status == "ready" - ) + select(func.coalesce(func.sum(MediaAsset.size_bytes), 0)).where(quota.ON_DISK) ).scalar() or 0 ) if total > cap: + # An upload counts toward the total (it is disk like any other) but is never evicted: + # it has no source to fetch again, and it lives until its owner deletes it. candidates = ( db.execute( select(MediaAsset) - .where(MediaAsset.status == "ready") + .where(MediaAsset.status == "ready", MediaAsset.source_kind != "upload") .order_by(MediaAsset.last_access_at.asc().nulls_first()) ) .scalars() @@ -232,10 +240,10 @@ def run_download_gc(db: Session) -> dict: if orphans: db.commit() - # 4b. Reap unheld uploads. An upload carries no expires_at and is never re-enqueued, so the passes - # above never free one that lost its last holder outside `release_asset` — a conversion that - # finished after its job was cancelled, or one cancelled mid-run (the asset was `downloading` - # then). Its file, ready or still the unconverted original, sits at `rel_path`. + # 4b. Reap unheld uploads. An upload carries no expires_at, is never evicted and never re-enqueued, + # so the passes above never free one that lost its last holder outside `release_asset` — a + # conversion that finished after its job was cancelled, or one cancelled mid-run (the asset + # was `downloading` then). Its file, ready or still the unconverted original, is at `rel_path`. unheld = ( db.execute( select(MediaAsset).where( diff --git a/backend/app/downloads/pipeline.py b/backend/app/downloads/pipeline.py index 3d3510ea..853a3b5d 100644 --- a/backend/app/downloads/pipeline.py +++ b/backend/app/downloads/pipeline.py @@ -59,6 +59,10 @@ _DB_WRITE_INTERVAL_S = 1.0 _CANCEL_POLL_INTERVAL_S = 1.0 _THUMB_SUFFIXES = (".jpg", ".jpeg", ".png", ".webp") +# The job phase while a file is brought into the phone-safe shape (`_ensure_phone_safe`) for a +# download or an upload. Its own word, not yt-dlp's "processing": it reports ffmpeg's real progress, +# so the queue row shows a % for it, where yt-dlp's post-steps only get an indeterminate pulse. +CONVERTING = "converting" # yt-dlp's working files in the staging dir. None of them is ever the finished media, but several # CAN be the biggest file there (a video-only per-format stream, an abandoned fragment run), so the # staging fallback must exclude them by name — picking one would store e.g. a silent video-only @@ -76,7 +80,7 @@ def _notify_download_done(db, user_id: int | None, title: str | None, job_id, *, """Post a best-effort 'ready' notice to the owner's inbox — downloads/edits used to finish silently while scheduler jobs and playlist-push already notified (feedback symmetry). A notification failure must never mask the completed job, so it is isolated + rolled back (mirrors - scheduler._notify_done). `kind` ("download"/"edit") lets the inbox pick accurate copy; the + scheduler._notify_done). `kind` ("download"/"edit"/"upload") lets the inbox pick accurate copy; the English title here is only a fallback for an unknown type. `user_id`/`title` are captured by the caller BEFORE its commit, so reading them here needs no post-commit refresh of an expired row.""" if user_id is None: @@ -529,7 +533,7 @@ def _download( job_id, produced, staging, - "processing", + CONVERTING, int(info["duration"]) if info.get("duration") else None, ) ext = produced.suffix.lstrip(".").lower() @@ -751,7 +755,9 @@ def _fail_edit( job_id, status=JobStatus.ERROR, error=msg, - error_code=dlerrors.classify(msg), + error_code=dlerrors.classify_conversion(msg) + if what == "upload" + else dlerrors.classify(msg), phase=None, ) db.commit() @@ -833,6 +839,8 @@ def _process_edit(job_id: int, asset_id: int) -> None: storage.place_file(dest_staging, settings.download_root, rel) final = storage.abs_path(settings.download_root, rel) size = final.stat().st_size if final.exists() else None + # The clip's real frame: a re-encode evens an odd crop or source (`editmod.EVEN_FRAME`). + _, _, out_w, out_h, _ = probe.probe_media(final) _, retention_days = _download_settings() expires = datetime.now(timezone.utc) + timedelta(days=retention_days) @@ -844,7 +852,9 @@ def _process_edit(job_id: int, asset_id: int) -> None: asset.size_bytes = size asset.container = out_ext asset.duration_s = total_s or asset.duration_s - if crop: + if out_w and out_h: + asset.width, asset.height = out_w, out_h + elif crop: asset.width = edit_spec["crop"]["w"] asset.height = edit_spec["crop"]["h"] else: @@ -886,29 +896,57 @@ def _process_edit(job_id: int, asset_id: int) -> None: shutil.rmtree(staging, ignore_errors=True) +def _check_converted_fits(owner_id: int | None, old_size: int, new_size: int) -> None: + """Refuse a converted upload that would push its owner over the storage cap: the upload was + charged at its original size, and a re-encode can come out larger. The original stays, counted, + for the owner to delete or keep.""" + if owner_id is None: + return + with SessionLocal() as db: + if not quota.fits_after_conversion(db, owner_id, old_size, new_size): + raise StatedFailure(dlerrors.QUOTA_FULL_MESSAGE) + + +def _unplace(placed: Path | None) -> None: + """Remove a converted upload that was placed at its new rel but never committed: the row still + names the original, so nothing else would ever find this file.""" + if placed is not None: + placed.unlink(missing_ok=True) + + def _convert_upload(job_id: int, asset_id: int) -> None: """Convert a queued upload into the phone-safe shape (`playable`) and make it ready. The original sits at the asset's `rel_path` (see `upload`); the converted file replaces it there under the new extension, and only then is the original removed — so a failed or stopped - conversion leaves the original in place for Retry, and a delete still finds it by `rel_path`.""" + conversion leaves the original in place for Retry, and a delete still finds it by `rel_path`. + A converted file placed but never committed is removed again on the way out; a stop in the + instant between the commit and the original's unlink is left to the GC's `.uploads` sweep.""" with SessionLocal() as db: asset = db.get(MediaAsset, asset_id) old_rel = asset.rel_path if asset else None + old_ext = asset.container if asset else None + old_size = (asset.size_bytes or 0) if asset else 0 duration = asset.duration_s if asset else None + job = db.get(DownloadJob, job_id) + owner_id = job.user_id if job else None src = storage.safe_abs_path(settings.download_root, old_rel) if old_rel else None if old_rel is None or src is None or not src.exists(): - _fail_edit(job_id, asset_id, "The uploaded file is missing.", what="upload") + _fail_edit(job_id, asset_id, dlerrors.UPLOAD_MISSING_MESSAGE, what="upload") return staging = _staging_dir(asset_id) + placed: Path | None = None # the converted file while it is at its new rel, uncommitted try: shutil.rmtree(staging, ignore_errors=True) staging.mkdir(parents=True, exist_ok=True) - safe = _ensure_phone_safe(job_id, src, staging, "processing", duration, keep_source=True) + safe = _ensure_phone_safe(job_id, src, staging, CONVERTING, duration, keep_source=True) ext = safe.suffix.lstrip(".").lower() - new_rel = Path(old_rel).with_suffix(f".{ext}").as_posix() + new_rel = storage.with_ext(old_rel, old_ext, ext) if safe != src: + _check_converted_fits(owner_id, old_size, safe.stat().st_size) storage.place_file(safe, settings.download_root, new_rel) + if new_rel != old_rel: + placed = storage.abs_path(settings.download_root, new_rel) final = storage.abs_path(settings.download_root, new_rel) vcodec, acodec, width, height, dur = probe.probe_media(final) with SessionLocal() as db: @@ -923,15 +961,14 @@ def _convert_upload(job_id: int, asset_id: int) -> None: asset.duration_s = dur or asset.duration_s asset.error = None asset.last_access_at = datetime.now(timezone.utc) - job = db.get(DownloadJob, job_id) - owner_id = job.user_id if job else None title = asset.title if asset else None finalized = service.finalize_if_running( db, job_id, status=JobStatus.DONE, progress=100, phase=None ) db.commit() + placed = None if finalized: - _notify_download_done(db, owner_id, title, job_id, kind="download") + _notify_download_done(db, owner_id, title, job_id, kind="upload") if new_rel != old_rel: src.unlink(missing_ok=True) if finalized: @@ -943,6 +980,7 @@ def _convert_upload(job_id: int, asset_id: int) -> None: ) except editmod.EditAborted: # Paused/canceled mid-encode: the original is untouched; free the asset for a later retry. + _unplace(placed) _reset_asset_pending(asset_id) log.info( "upload job %s conversion aborted (pause/cancel)", @@ -951,6 +989,8 @@ def _convert_upload(job_id: int, asset_id: int) -> None: ) except Exception as exc: # noqa: BLE001 — surface the failure, keep the worker alive # silent-ok: not silent — `_fail_edit` is the report, and it takes `exc` for its stack. + _unplace(placed) _fail_edit(job_id, asset_id, str(exc)[:500], exc, what="upload") + finally: shutil.rmtree(staging, ignore_errors=True) diff --git a/backend/app/downloads/playable.py b/backend/app/downloads/playable.py index 8de66bcb..ec0381c4 100644 --- a/backend/app/downloads/playable.py +++ b/backend/app/downloads/playable.py @@ -80,6 +80,7 @@ def build_cmd(src: Path, dest: Path, p: Plan, threads: int | None = None) -> lis "-progress", "pipe:1", "-nostats", + *editmod.INPUT_GUARD, "-i", str(src), ] @@ -87,7 +88,10 @@ def build_cmd(src: Path, dest: Path, p: Plan, threads: int | None = None) -> lis # else rides along — a subtitle or data track the .mp4 muxer cannot take would fail the run. if p.has_video: cmd += ["-map", "0:V:0", "-map", "0:a:0?"] - cmd += editmod.X264_ARGS if p.reencode_video else ["-c:v", "copy"] + if p.reencode_video: + cmd += ["-vf", editmod.EVEN_FRAME, *editmod.X264_ARGS] + else: + cmd += ["-c:v", "copy"] else: cmd += ["-map", "0:a:0"] cmd += editmod.AAC_ARGS if p.reencode_audio else ["-c:a", "copy"] diff --git a/backend/app/downloads/quota.py b/backend/app/downloads/quota.py index 2fcd174e..66c92ef8 100644 --- a/backend/app/downloads/quota.py +++ b/backend/app/downloads/quota.py @@ -1,7 +1,9 @@ """Per-user download limits + footprint accounting. A user's *footprint* is the total size of the ready assets their library references (cache hits -count — it's "how much disk you're responsible for", not "how much you personally downloaded"). +count — it's "how much disk you're responsible for", not "how much you personally downloaded"). An +upload counts from the moment it lands, ready or not: its original is on disk while its conversion +waits, and stays there when the conversion fails (for Retry) — bytes a ready-only sum never saw. Limits come from a per-user DownloadQuota row if the admin set one, else the sysconfig defaults. `unlimited` (e.g. the admin's own account) bypasses the byte cap. @@ -18,16 +20,24 @@ is unknown until it finishes. Edit jobs do not consult it at all (tracked as SEC from dataclasses import dataclass -from sqlalchemy import func, select +from sqlalchemy import and_, func, or_, select from sqlalchemy.orm import Session from app import sysconfig +from app.downloads.status import JobStatus from app.models import DownloadJob, DownloadQuota, MediaAsset # Jobs a user is considered to be "holding" (occupy a job slot / footprint). Terminal failures # (error, canceled) don't count. -_HOLDING = ("queued", "running", "paused", "done") -_RUNNING = ("queued", "running") +_HOLDING = (JobStatus.QUEUED, JobStatus.RUNNING, JobStatus.PAUSED, JobStatus.DONE) +_RUNNING = (JobStatus.QUEUED, JobStatus.RUNNING) +# An asset whose file is on disk: a ready one, or an upload in any state (its original lands at +# ingest, see the module docstring). Every byte sum — a user's footprint, the GC's total-cap pass, +# the admin's storage view — reads it, so they all count the same disk. +ON_DISK = or_( + MediaAsset.status == "ready", + and_(MediaAsset.source_kind == "upload", MediaAsset.rel_path.is_not(None)), +) @dataclass @@ -67,12 +77,17 @@ def resolve(db: Session, user_id: int) -> QuotaLimits: def footprint(db: Session, user_id: int) -> int: - """Total bytes of the distinct ready assets this user's active jobs reference.""" + """Total bytes of the distinct assets this user's jobs hold on disk: the ready ones their active + jobs reference, and every upload of theirs — including one whose conversion failed, which keeps + its original (see the module docstring).""" held_assets = ( select(DownloadJob.asset_id) .where( DownloadJob.user_id == user_id, - DownloadJob.status.in_(_HOLDING), + or_( + DownloadJob.status.in_(_HOLDING), + and_(DownloadJob.job_kind == "upload", DownloadJob.status == JobStatus.ERROR), + ), DownloadJob.asset_id.is_not(None), ) .distinct() @@ -80,7 +95,7 @@ def footprint(db: Session, user_id: int) -> int: return int( db.execute( select(func.coalesce(func.sum(MediaAsset.size_bytes), 0)).where( - MediaAsset.id.in_(held_assets), MediaAsset.status == "ready" + MediaAsset.id.in_(held_assets), ON_DISK ) ).scalar() or 0 @@ -111,7 +126,7 @@ def at_concurrency_limit(db: Session, user_id: int) -> bool: lim = resolve(db, user_id) if lim.unlimited: return False - running = _count(db, user_id, ("running",)) + running = _count(db, user_id, (JobStatus.RUNNING,)) return running >= lim.max_concurrent @@ -160,6 +175,18 @@ def check_upload(db: Session, user_id: int, add_bytes: int) -> None: raise QuotaExceeded("max_bytes", lim.max_bytes, projected) +def fits_after_conversion(db: Session, user_id: int, old_bytes: int, new_bytes: int) -> bool: + """Whether an upload converted from `old_bytes` (what it is charged at now) to `new_bytes` keeps + its owner within the cap. A re-encode can come out larger than the original, and nothing else + re-checks it once the upload was let in.""" + if new_bytes <= old_bytes: # a smaller file never breaks a cap the larger one was let in under + return True + lim = resolve(db, user_id) + if lim.unlimited: + return True + return footprint(db, user_id) - old_bytes + new_bytes <= lim.max_bytes + + def usage(db: Session, user_id: int) -> dict: """Compact usage snapshot for the UI (footprint + counts vs limits).""" lim = resolve(db, user_id) diff --git a/backend/app/downloads/storage.py b/backend/app/downloads/storage.py index 0084938e..4a3ea17e 100644 --- a/backend/app/downloads/storage.py +++ b/backend/app/downloads/storage.py @@ -168,13 +168,25 @@ def upload_rel_path(user_id: int, asset_id: int, filename: str) -> str: episode sidecars). The name is system-decided and bound to the asset id (`_a{id}` keeps two same-named uploads apart); the original filename is display metadata only. The extension is preserved (shell-safe, bounded) so the served file + its Content-Disposition carry a correct - suffix; an extensionless upload stays extensionless.""" - stem = sanitize(Path(filename).stem, 80) + suffix; an extensionless upload stays extensionless. + + The stem holds no dot, so the last dot of the name is always the extension's: every sibling + derived by swapping it (`with_suffix` — the poster, the converted file) keeps `_a{id}`. A dotted + stem with no extension ('Holiday.2024.ä') once lost it, and two such uploads shared one file.""" + stem = sanitize(Path(filename).stem, 80).replace(".", "_") ext = _clean_upload_ext(filename) suffix = f".{ext}" if ext else "" return f".uploads/{user_id}/{stem}_a{asset_id}{suffix}" +def with_ext(rel: str, old_ext: str | None, ext: str) -> str: + """`rel` with its extension — `old_ext`, the stored `container`, None for a bare name — swapped + for `ext`. Not `Path.with_suffix`: on a bare name whose stem holds a dot (an upload placed before + the stems lost their dots) that would take the stem's tail for the extension, and `_a{id}` with it.""" + base = rel[: -len(old_ext) - 1] if old_ext and rel.endswith(f".{old_ext}") else rel + return f"{base}.{ext}" + + def upload_ext(filename: str) -> str | None: """The stored `container` for an uploaded file: its shell-safe lowercased extension, or None when the upload has no extension (a bare name). Shares `_clean_upload_ext` with `upload_rel_path` so the diff --git a/backend/app/downloads/upload.py b/backend/app/downloads/upload.py index e7ebd443..7a491ea7 100644 --- a/backend/app/downloads/upload.py +++ b/backend/app/downloads/upload.py @@ -10,9 +10,11 @@ now" shortcut `service.enqueue` takes on a cache hit. Media that is NOT phone-safe (an iPhone's HEVC .mov, a VP9 .webm, an .mkv) is placed as it came and queued: the asset stays `pending` and the job `queued`, and the download worker converts it (`pipeline._convert_upload`) — so a share link never hands a friend a file their phone cannot play. -Only the converted file is kept (the owner's choice: one file per upload; a 10-bit HDR original -becomes 8-bit SDR). Until then the original sits at the asset's `rel_path`, which is what a failed -conversion's Retry starts from again, and what a delete removes (`service.release_asset`). +Only the converted file is kept (the owner's choice: one file per upload). A 10-bit HDR original +becomes 8-bit H.264 with its colour tags kept — still tagged HDR (HLG/PQ, BT.2020), not tone-mapped +to SDR (the owner's choice too). Until then the original sits at the asset's `rel_path`, which is +what a failed conversion's Retry starts from again, and what a delete removes +(`service.release_asset`). Kinds (the invariant lives in `mediakind`): a probe with a real video stream -> video, audio-only -> audio, neither -> a generic 'file' (pdf/zip/docx/…) that is download-only and never rendered @@ -63,7 +65,10 @@ def ingest_upload( data = probe.probe_json(src_path) vcodec, acodec, width, height, duration = probe.facts(data) kind = mediakind.derive(vcodec, acodec) - safe = playable.plan(data, Path(filename).suffix) if kind != mediakind.FILE else None + # Judged by the extension the file is PLACED under (and the worker re-judges), not the raw one: + # 'clip.mp4~' is stored as .mp4, and judging 'mp4~' queued a conversion with nothing to convert. + ext = storage.upload_ext(filename) or "" + safe = playable.plan(data, ext) if kind != mediakind.FILE else None convert = safe is not None and safe.needs_change # An upload never dedups (each file is its own asset): a random ref makes the cache key unique, @@ -88,7 +93,7 @@ def ingest_upload( asset.status = "pending" if convert else "ready" asset.rel_path = rel asset.size_bytes = actual_size - asset.container = storage.upload_ext(filename) + asset.container = ext or None asset.vcodec = vcodec asset.acodec = acodec asset.width = width diff --git a/backend/app/models.py b/backend/app/models.py index 608bc46d..4c799a33 100644 --- a/backend/app/models.py +++ b/backend/app/models.py @@ -912,7 +912,8 @@ class DownloadJob(Base, TimestampMixin, UpdatedAtMixin): ) source_kind: Mapped[str] = mapped_column(String(16)) # "youtube" | "url" source_ref: Mapped[str] = mapped_column(String(512)) - # "download" (default) or "edit" (a per-user trim/crop derivative of another job). + # "download" (default), "edit" (a per-user trim/crop derivative of another job) or "upload" (a + # user's own file, converted by the worker when it is not phone-safe — see `downloads.upload`). job_kind: Mapped[str] = mapped_column(String(16), default="download", server_default="download") # For edit jobs: the parent download this clip is cut from (SET NULL if the parent is deleted). source_job_id: Mapped[int | None] = mapped_column( diff --git a/backend/app/routes/downloads.py b/backend/app/routes/downloads.py index 49e656dc..d9a81c19 100644 --- a/backend/app/routes/downloads.py +++ b/backend/app/routes/downloads.py @@ -32,6 +32,7 @@ from app.downloads import links as linksmod from app.downloads import quota, service, storage from app.downloads import upload as uploadmod from app.downloads import urlguard +from app.downloads.status import JobStatus from app.models import ( DownloadJob, DownloadLink, @@ -131,8 +132,8 @@ def _serialize(job: DownloadJob, asset: MediaAsset | None) -> dict: "job_kind": job.job_kind, "source_job_id": job.source_job_id, "edit_spec": job.edit_spec, - "can_download": ready and job.status == "done", - "expired": job.asset_id is None and job.status == "done", + "can_download": ready and job.status == JobStatus.DONE, + "expired": job.asset_id is None and job.status == JobStatus.DONE, "asset": None if asset is None else { @@ -540,7 +541,7 @@ def enqueue_edit( clip is a NEW per-user derivative in the editor's own library (counts against their quota); the source file is only read, never modified, so editing a shared video is safe.""" source_job = _accessible_job(db, user, body.source_job_id) - if source_job.status != "done" or source_job.asset_id is None: + if source_job.status != JobStatus.DONE or source_job.asset_id is None: raise refused(409, "edit_unfinished") spec = body.edit_spec display_name = display_text(body.display_name) @@ -594,7 +595,12 @@ def my_usage(user: User = Depends(current_user), db: Session = Depends(get_db)) # Priority when a source has several jobs (e.g. a done one plus a fresh queued one). -_INDEX_RANK = {"done": 4, "running": 3, "paused": 2, "queued": 1} +_INDEX_RANK: dict[str, int] = { + JobStatus.DONE: 4, + JobStatus.RUNNING: 3, + JobStatus.PAUSED: 2, + JobStatus.QUEUED: 1, +} @router.get("/index") @@ -604,7 +610,7 @@ def my_download_index(user: User = Depends(current_user), db: Session = Depends( rows = db.execute( select(DownloadJob.source_ref, DownloadJob.status).where( DownloadJob.user_id == user.id, - DownloadJob.status.in_(("queued", "running", "paused", "done")), + DownloadJob.status.in_(tuple(_INDEX_RANK)), ) ).all() out: dict[str, str] = {} @@ -735,7 +741,7 @@ def redownload_download( # Neither has a re-fetchable source: an editor clip is derived locally, an upload IS the # source. Re-running yt-dlp against source_kind='upload'/'edit' would just fail. raise refused(409, "not_redownloadable") - if job.status in ("queued", "running", "paused"): + if job.status in (JobStatus.QUEUED, JobStatus.RUNNING, JobStatus.PAUSED): raise refused(409, "still_downloading") service.redownload(db, job) return _serialize_job(db, job) @@ -887,7 +893,7 @@ def share_download( db: Session = Depends(get_db), ) -> dict: job = owned_or_404(db, DownloadJob, job_id, user, what="download") - if job.status != "done" or job.asset_id is None: + if job.status != JobStatus.DONE or job.asset_id is None: raise refused(409, "share_unfinished") target = body.email.strip().lower() if not target: @@ -1051,7 +1057,7 @@ def create_link( db: Session = Depends(get_db), ) -> dict: job = owned_or_404(db, DownloadJob, job_id, user, what="download") - if job.status != "done" or job.asset_id is None: + if job.status != JobStatus.DONE or job.asset_id is None: raise refused(409, "share_unfinished") pw = body.password.strip() link = DownloadLink( @@ -1139,9 +1145,7 @@ def admin_list_downloads( @admin_router.get("/storage") def admin_storage(admin: User = Depends(admin_user), db: Session = Depends(get_db)) -> dict: ready = db.execute( - select(func.count(), func.coalesce(func.sum(MediaAsset.size_bytes), 0)).where( - MediaAsset.status == "ready" - ) + select(func.count(), func.coalesce(func.sum(MediaAsset.size_bytes), 0)).where(quota.ON_DISK) ).one() total_assets = db.execute(select(func.count()).select_from(MediaAsset)).scalar() per_user = db.execute( diff --git a/backend/tests/test_ffmpeg_input_guard.py b/backend/tests/test_ffmpeg_input_guard.py new file mode 100644 index 00000000..d87cbf2e --- /dev/null +++ b/backend/tests/test_ffmpeg_input_guard.py @@ -0,0 +1,163 @@ +"""Every ffmpeg/ffprobe the downloads package runs over a library file names the demuxers it may use +(`edit.INPUT_GUARD`, a downloaded thumbnail's `edit.THUMB_GUARD`). + +Without the list, ffmpeg picks a demuxer from the file's content and name, and both are the user's +for an upload (the remote site's for a thumbnail): a playlist-type demuxer opens whatever local paths +its entries name. These tests read the argv each builder hands to ffmpeg — the guard is a flag, so +the flag is what is held — and scan the package for an ffmpeg/ffprobe call written without one. + +Not in scope: `app/plex`, whose ffmpeg reads the admin's own Plex library, not user-supplied bytes. +""" + +import ast +import shutil +import subprocess +from pathlib import Path + +import pytest + +from app.downloads import edit, playable + +DOWNLOADS = Path(__file__).resolve().parents[1] / "app" / "downloads" +# The demuxers that read paths named inside a file. `concat` is allowed only where the list is ours. +PLAYLIST_DEMUXERS = {"hls", "dash", "concat", "image2", "ffconcat", "tee"} +MEDIA = set(edit.MEDIA_DEMUXERS) + + +def _whitelists(cmd: list[str]) -> list[set[str] | None]: + """For each `-i` in `cmd`, the demuxers the `-format_whitelist` given since the previous input + allows — None when that input has none.""" + out, current = [], None + for i, tok in enumerate(cmd): + if tok == "-format_whitelist": + current = set(cmd[i + 1].split(",")) + elif tok == "-i": + out.append(current) + current = None + return out + + +def _assert_media_guarded(cmd: list[str], extra: set[str] = frozenset()) -> None: + lists = _whitelists(cmd) + assert lists, cmd # a builder that opens no input is not what this test is about + for allowed in lists: + assert allowed is not None, f"an input with no -format_whitelist: {cmd}" + assert allowed <= MEDIA | extra, allowed - MEDIA - extra + assert not (allowed - extra) & PLAYLIST_DEMUXERS + + +def test_the_media_list_names_no_demuxer_that_reads_other_paths(): + assert not MEDIA & PLAYLIST_DEMUXERS + assert edit.INPUT_GUARD == ["-format_whitelist", ",".join(edit.MEDIA_DEMUXERS)] + + +def test_the_phone_safe_conversion_is_guarded(): + p = playable.Plan("mp4", True, True, True, True) + _assert_media_guarded(playable.build_cmd(Path("in.mov"), Path("out.mp4"), p)) + + +@pytest.mark.parametrize( + "spec", + [ + {"trim": {"start_s": 1, "end_s": 3}, "accurate": False}, + {"trim": {"start_s": 1, "end_s": 3}, "accurate": True}, + {"crop": {"x": 0, "y": 0, "w": 101, "h": 51}}, + ], + ids=["fast trim", "accurate trim", "crop"], +) +def test_a_clip_is_guarded(spec): + spec = edit.normalize_edit_spec(spec) + _assert_media_guarded(edit.build_edit_cmd(Path("in.mp4"), Path("out.mp4"), spec, "mp4")) + + +@pytest.mark.parametrize("accurate", [True, False], ids=["accurate join", "fast join"]) +def test_a_join_is_guarded(accurate, tmp_path): + segs = [{"start_s": 0, "end_s": 1}, {"start_s": 2, "end_s": 3}] + spec = edit.normalize_edit_spec({"segments": segs, "accurate": accurate}) + cmd, _ = edit.build_concat_plan(Path("in.mp4"), Path("out.mp4"), spec, "mp4", tmp_path) + # The fast join reads a concat list WE write; the whitelist rides down to the file it names. + _assert_media_guarded(cmd, extra=set() if accurate else {"concat"}) + + +def test_the_poster_and_the_filmstrip_are_guarded(): + _assert_media_guarded(edit.build_poster_cmd(Path("in.mp4"), Path("p.jpg"), 2.0)) + geom = edit.storyboard_geometry(60) + _assert_media_guarded(edit.build_storyboard_cmd(Path("in.mp4"), Path("s.jpg"), 60, geom)) + + +def test_a_downloaded_thumbnail_opens_as_a_single_image_only(): + (allowed,) = _whitelists(edit.build_thumbnail_jpg_cmd(Path("t.webp"), Path("t.jpg"))) + assert allowed == {"png_pipe", "jpeg_pipe", "webp_pipe"} + + +def test_every_ffprobe_is_guarded(monkeypatch): + seen = [] + + class _Done: + stdout = "{}" + + def _run(args, **_k): + seen.append(args) + return _Done() + + monkeypatch.setattr(edit.subprocess, "run", _run) + edit.run_ffprobe_json(Path("x.mp4"), show_streams=True) + edit.probe_duration(Path("x.mp4")) + assert len(seen) == 2 + for args in seen: + i = args.index("-format_whitelist") + assert set(args[i + 1].split(",")) == MEDIA + + +def _unguarded_calls(source: str) -> list[int]: + """Lines of a list literal that starts an ffmpeg/ffprobe command or hands one an input (`-i`) + and carries no `*…GUARD` / `*guard` splat and no "-format_whitelist" of its own. A builder that + adds its input in a later `+=` must guard that list — that is where the input is.""" + bad = [] + for node in ast.walk(ast.parse(source)): + if not isinstance(node, ast.List): + continue + consts = {e.value for e in node.elts if isinstance(e, ast.Constant)} + if not consts & {"-i", "ffprobe"}: + continue + guarded = "-format_whitelist" in consts or any( + isinstance(e, ast.Starred) + and isinstance(v := e.value, ast.Name | ast.Attribute) + and (v.id if isinstance(v, ast.Name) else v.attr).upper().endswith("GUARD") + for e in node.elts + ) + if not guarded: + bad.append(node.lineno) + return bad + + +def test_no_ffmpeg_call_in_the_package_opens_an_input_unguarded(): + found = { + f"{p.name}:{line}" + for p in sorted(DOWNLOADS.rglob("*.py")) + for line in _unguarded_calls(p.read_text(encoding="utf-8")) + } + assert found == set() + + +def test_the_scan_sees_an_unguarded_call(): + # The scan above is green over nothing if it cannot see one: both shapes it must catch. + assert _unguarded_calls('cmd = ["ffmpeg", "-y", "-i", src]') == [1] + assert _unguarded_calls('a = ["ffprobe", "-v", "error", str(p)]') == [1] + assert _unguarded_calls('cmd = [*INPUT_GUARD, "-i", src]') == [] + + +@pytest.mark.skipif(shutil.which("ffmpeg") is None, reason="ffmpeg not available") +@pytest.mark.parametrize("ext", ["png", "webp", "jpg"]) +def test_the_thumbnail_guard_still_opens_every_thumbnail_a_download_keeps(ext, tmp_path): + # `pipeline._THUMB_SUFFIXES`: what the guard must still let through (a .jpg that is really a + # PNG goes the ffmpeg route too — `thumbnail_to_jpg` copies only true JPEG bytes). + src = tmp_path / f"t.{ext}" + fmt = ["-f", "image2", "-c:v", "png"] if ext == "jpg" else [] + subprocess.run( + ["ffmpeg", "-y", "-loglevel", "error", "-f", "lavfi", "-i", "testsrc2=size=64x48", + "-frames:v", "1", *fmt, str(src)], + check=True, + ) # fmt: skip + out = edit.thumbnail_to_jpg(src, tmp_path / "poster.jpg") + assert out is not None and edit._is_jpeg(out) diff --git a/backend/tests/test_odd_frame.py b/backend/tests/test_odd_frame.py new file mode 100644 index 00000000..13f13561 --- /dev/null +++ b/backend/tests/test_odd_frame.py @@ -0,0 +1,73 @@ +"""A libx264 re-encode of an odd-sized frame: 4:2:0 halves the chroma, so libx264 refuses an odd +width or height ("width not divisible by 2"). Measured: a source that is odd AND not 4:2:0 already +(a 4:4:4 or RGB screen capture, 1365x767) failed its conversion outright; a 4:2:0 source never +reaches the encoder odd (ffmpeg's crop rounds to its chroma grid), but the clip then stored the +size the user dragged, not the one the file has. Every re-encode now ends its filter chain on +`edit.EVEN_FRAME` (at most one pixel row or column), and a clip stores its probed frame. + +Real ffmpeg on both paths — the refusal is the encoder's, so only the encoder can show it is gone. +""" + +import shutil +import subprocess +from pathlib import Path + +import pytest + +from app.config import settings +from app.downloads import pipeline, playable, probe, service, storage +from app.downloads import upload as uploadmod +from app.models import DownloadJob, MediaAsset + +pytestmark = pytest.mark.skipif(shutil.which("ffmpeg") is None, reason="ffmpeg not available") + + +def _ffmpeg(*args: str) -> None: + subprocess.run(["ffmpeg", "-y", "-loglevel", "error", *args], check=True) + + +def _frame(path: Path) -> tuple[int | None, int | None]: + _, _, w, h, _ = probe.probe_media(path) + return w, h + + +def test_an_odd_sized_4_4_4_video_converts_to_the_even_frame_below_it(tmp_path): + src = tmp_path / "in.mkv" + _ffmpeg( + "-f", "lavfi", "-i", "testsrc=size=161x121:rate=10:duration=1", + "-c:v", "libx264", "-pix_fmt", "yuv444p", str(src), + ) # fmt: skip + assert _frame(src) == (161, 121) # odd for real (`testsrc2` rounds its own size) + p = playable.plan(probe.probe_json(src), "mkv") + assert p is not None and p.reencode_video + dest = tmp_path / "out.mp4" + subprocess.run(playable.build_cmd(src, dest, p), check=True, capture_output=True) + assert _frame(dest) == (160, 120) + + +def test_an_odd_crop_makes_a_clip_and_stores_the_frame_it_has(db, make_user, tmp_path, monkeypatch): + monkeypatch.setattr(settings, "download_root", str(tmp_path)) + src = tmp_path / "in.mp4" + _ffmpeg( + "-f", "lavfi", "-i", "testsrc2=size=160x120:rate=10:duration=2", + "-f", "lavfi", "-i", "sine=duration=2", + "-c:v", "libx264", "-pix_fmt", "yuv420p", "-c:a", "aac", str(src), + ) # fmt: skip + u = make_user() + source = uploadmod.ingest_upload( + db, u.id, filename="clip.mp4", src_path=src, size=src.stat().st_size + ) + assert source.status == "done" # already phone-safe: the clip's source as it came + job = service.enqueue_edit(db, u.id, source, {"crop": {"x": 3, "y": 5, "w": 101, "h": 51}}) + asset = db.get(MediaAsset, job.asset_id) + job.status, asset.status = "running", "downloading" # the worker's claim + db.commit() + + pipeline._process_edit(job.id, asset.id) + + db.expire_all() + job, asset = db.get(DownloadJob, job.id), db.get(MediaAsset, job.asset_id) + assert (job.status, job.error, asset.status) == ("done", None, "ready") + final = storage.abs_path(str(tmp_path), asset.rel_path) + assert _frame(final) == (100, 50) + assert (asset.width, asset.height) == (100, 50) # what the file is, not what was asked diff --git a/backend/tests/test_upload_conversion.py b/backend/tests/test_upload_conversion.py new file mode 100644 index 00000000..e7452881 --- /dev/null +++ b/backend/tests/test_upload_conversion.py @@ -0,0 +1,355 @@ +"""An upload after ingest: what it costs its owner from the moment it lands, how the worker's +conversion names, checks and cleans up the file it makes, and what the owner and a share link see. + +DB-backed. The conversion's ffmpeg step (`pipeline._ensure_phone_safe`) is stubbed to write a file of +a chosen size — these tests are about what happens around the encode; `test_upload.py` runs the real +one, and `test_playable.py` judges its bytes. +""" + +import os +import time +from datetime import datetime, timedelta, timezone +from pathlib import Path + +import pytest + +from app import sysconfig +from app.config import settings +from app.downloads import gc as gcmod +from app.downloads import pipeline, quota, storage +from app.downloads import probe as probemod +from app.downloads import upload as uploadmod +from app.downloads.stated import StatedFailure +from app.models import DownloadJob, DownloadQuota, MediaAsset, Notification +from sqlalchemy import select + + +@pytest.fixture +def _root(tmp_path, monkeypatch): + monkeypatch.setattr(settings, "download_root", str(tmp_path)) + return tmp_path + + +def _probe(monkeypatch, vcodec="hevc"): + """ffprobe's view of every file here: `vcodec` + AAC, 8-bit 4:2:0 — HEVC is queued for the + worker, H.264 lands as it came.""" + data = { + "streams": [ + {"codec_type": "video", "codec_name": vcodec, "pix_fmt": "yuv420p", + "width": 640, "height": 360}, + {"codec_type": "audio", "codec_name": "aac"}, + ], + "format": {"format_name": "mov,mp4", "duration": "6"}, + } # fmt: skip + monkeypatch.setattr(probemod, "probe_json", lambda _p: data) + + +def _ingest(db, root, user_id, name="clip.mov", size=10): + src = root / "incoming.bin" + src.write_bytes(b"o" * size) + job = uploadmod.ingest_upload(db, user_id, filename=name, src_path=src, size=size) + return job, db.get(MediaAsset, job.asset_id) + + +def _claim(db, job, asset): + """What the worker's claim does before it converts.""" + job.status, asset.status = "running", "downloading" + db.commit() + + +def _converts_to(monkeypatch, size, phases=None): + """The encode writes a `size`-byte .mp4 into its staging dir (and notes the phase it reports).""" + + def _encode(job_id, produced, out_dir, phase, total_s=None, **_k): + if phases is not None: + phases.append(phase) + out = Path(out_dir) / "converted.mp4" + out.write_bytes(b"c" * size) + return out + + monkeypatch.setattr(pipeline, "_ensure_phone_safe", _encode) + + +def _cap(db, user_id, max_bytes): + db.add( + DownloadQuota( + user_id=user_id, max_bytes=max_bytes, max_concurrent=2, max_jobs=50, unlimited=False + ) + ) + db.commit() + + +def _uploads_dir(root, user_id) -> Path: + return root / ".uploads" / str(user_id) + + +# --- what it costs --------------------------------------------------------------------------- + + +def test_a_queued_upload_counts_against_its_owner_from_the_moment_it_lands( + db, make_user, _root, monkeypatch +): + _probe(monkeypatch) + u = make_user() + job, asset = _ingest(db, _root, u.id, size=10) + assert (job.status, asset.status) == ("queued", "pending") + assert quota.footprint(db, u.id) == 10 # its original is on disk already + + +def test_a_failed_conversion_still_counts_its_kept_original(db, make_user, _root, monkeypatch): + _probe(monkeypatch) + u = make_user() + job, asset = _ingest(db, _root, u.id, size=10) + job.status, asset.status = "error", "error" # kept for Retry + db.commit() + assert storage.abs_path(str(_root), asset.rel_path).exists() + assert quota.footprint(db, u.id) == 10 + + +def test_a_conversion_that_outgrows_the_cap_is_refused_and_keeps_the_original( + db, make_user, _root, monkeypatch +): + _probe(monkeypatch) + u = make_user() + _cap(db, u.id, max_bytes=100) + job, asset = _ingest(db, _root, u.id, size=10) + original = storage.abs_path(str(_root), asset.rel_path) + _claim(db, job, asset) + _converts_to(monkeypatch, size=500) + + pipeline._convert_upload(job.id, asset.id) + + db.expire_all() + job = db.get(DownloadJob, job.id) + assert (job.status, job.error_code) == ("error", "quota_exceeded") + assert original.exists() + assert [p.name for p in _uploads_dir(_root, u.id).iterdir()] == [original.name] + + +def test_a_conversion_that_shrinks_passes_an_owner_at_the_cap(db, make_user, _root, monkeypatch): + _probe(monkeypatch) + u = make_user() + _cap(db, u.id, max_bytes=10) + job, asset = _ingest(db, _root, u.id, size=10) + _claim(db, job, asset) + _converts_to(monkeypatch, size=5) + + pipeline._convert_upload(job.id, asset.id) + + db.expire_all() + assert db.get(DownloadJob, job.id).status == "done" + assert db.get(MediaAsset, asset.id).size_bytes == 5 + + +# --- the converted file's name and its leftovers ----------------------------------------------- + + +def test_two_dotted_bare_names_keep_a_file_each(db, make_user, _root, monkeypatch): + # 'Holiday.2024.ä': its suffix is no extension (nothing shell-safe is left of it), so the stem's + # own dot once read as one — and the converted name lost `_a{id}`, the one thing keeping two apart. + _probe(monkeypatch) + u = make_user() + _converts_to(monkeypatch, size=5) + finals = [] + for _ in range(2): + job, asset = _ingest(db, _root, u.id, name="Holiday.2024.ä") + _claim(db, job, asset) + pipeline._convert_upload(job.id, asset.id) + db.expire_all() + asset = db.get(MediaAsset, asset.id) + assert asset.status == "ready" + assert asset.rel_path.endswith(f"_a{asset.id}.mp4"), asset.rel_path + finals.append(asset.rel_path) + assert finals[0] != finals[1] + assert all(storage.abs_path(str(_root), r).exists() for r in finals) + + +def test_a_conversion_failing_after_it_placed_its_file_removes_it( + db, make_user, _root, monkeypatch +): + _probe(monkeypatch) + u = make_user() + job, asset = _ingest(db, _root, u.id) + original = storage.abs_path(str(_root), asset.rel_path) + _claim(db, job, asset) + _converts_to(monkeypatch, size=5) + + def _probe_fails(_path): + raise RuntimeError("probe crashed") + + monkeypatch.setattr(pipeline.probe, "probe_media", _probe_fails) + pipeline._convert_upload(job.id, asset.id) + + db.expire_all() + assert db.get(DownloadJob, job.id).status == "error" + assert original.exists() # Retry starts from it + # The row still names the original: a converted file left beside it is one nothing would find. + assert [p.name for p in _uploads_dir(_root, u.id).iterdir()] == [original.name] + + +# --- what the owner is told ------------------------------------------------------------------ + + +def test_a_failed_encode_is_a_failed_conversion_not_a_download_step( + db, make_user, _root, monkeypatch +): + _probe(monkeypatch) + job, asset = _ingest(db, _root, make_user().id) + _claim(db, job, asset) + + def _ffmpeg_fails(*_a, **_k): + raise StatedFailure("ffmpeg failed (1): unsupported") + + monkeypatch.setattr(pipeline.editmod, "run_ffmpeg", _ffmpeg_fails) + pipeline._convert_upload(job.id, asset.id) + db.expire_all() + assert db.get(DownloadJob, job.id).error_code == "convert_failed" + + +def test_a_missing_original_says_so(db, make_user, _root, monkeypatch): + _probe(monkeypatch) + job, asset = _ingest(db, _root, make_user().id) + _claim(db, job, asset) + storage.abs_path(str(_root), asset.rel_path).unlink() + + pipeline._convert_upload(job.id, asset.id) + db.expire_all() + assert db.get(DownloadJob, job.id).error_code == "upload_missing" + + +def test_the_conversion_reports_its_own_phase(db, make_user, _root, monkeypatch): + _probe(monkeypatch) + job, asset = _ingest(db, _root, make_user().id) + _claim(db, job, asset) + phases = [] + _converts_to(monkeypatch, size=5, phases=phases) + pipeline._convert_upload(job.id, asset.id) + assert phases == ["converting"] # not yt-dlp's "processing": the queue row shows ffmpeg's % + + +def test_the_owner_is_told_their_upload_is_ready(db, make_user, _root, monkeypatch): + _probe(monkeypatch) + u = make_user() + job, asset = _ingest(db, _root, u.id) + _claim(db, job, asset) + _converts_to(monkeypatch, size=5) + pipeline._convert_upload(job.id, asset.id) + notes = db.execute(select(Notification).where(Notification.user_id == u.id)).scalars().all() + assert [n.data.get("kind") for n in notes] == ["upload"] + + +def test_an_extension_is_judged_as_it_is_stored(db, make_user, _root, monkeypatch): + # 'clip.mp4~' is stored as .mp4; judged by the raw 'mp4~' it queued a conversion of a file that + # was phone-safe already. + _probe(monkeypatch, vcodec="h264") + job, asset = _ingest(db, _root, make_user().id, name="clip.mp4~") + assert (job.status, asset.status, asset.container) == ("done", "ready", "mp4") + + +# --- the garbage collector --------------------------------------------------------------------- + + +def _aged(path: Path, hours: float = 5) -> None: + old = time.time() - hours * 3600 + os.utime(path, (old, old)) + + +def test_the_sweep_takes_a_stranded_upload_file_and_keeps_a_waiting_original( + db, make_user, _root, monkeypatch +): + _probe(monkeypatch) + u = make_user() + _job, asset = _ingest(db, _root, u.id) + original = storage.abs_path(str(_root), asset.rel_path) + stranded = original.with_name("left_by_a_stopped_conversion_a999.mp4") + stranded.write_bytes(b"x") + _aged(original) + _aged(stranded) + + gcmod._sweep_orphan_files(db, str(_root)) + + assert original.exists() # queued for conversion: its row names it, ready or not + assert not stranded.exists() # no row names it + + +def test_the_cache_cap_never_evicts_an_upload(db, make_user, _root, monkeypatch): + _probe(monkeypatch, vcodec="h264") + u = make_user() + _job, upload = _ingest(db, _root, u.id, name="mine.mp4", size=10) + db.add( + MediaAsset( + source_kind="youtube", source_ref="vid1", format_sig="best", status="ready", + rel_path="Chan/clip.mp4", size_bytes=10, title="downloaded", + last_access_at=datetime.now(timezone.utc), + ) + ) # fmt: skip + # The upload is the least recently used: an LRU pass that may take it takes it first. + upload.last_access_at = datetime.now(timezone.utc) - timedelta(days=30) + db.commit() + (_root / "Chan").mkdir() + (_root / "Chan" / "clip.mp4").write_bytes(b"d" * 10) + sysconfig.set_value(db, "download_total_max_bytes", 15) # 20 on disk + db.commit() + + gcmod.run_download_gc(db) + + db.expire_all() + assert db.get(MediaAsset, upload.id).status == "ready" + assert storage.abs_path(str(_root), upload.rel_path).exists() + + +# --- a share link ---------------------------------------------------------------------------- + + +@pytest.mark.parametrize("state", ["pending", "downloading"]) +def test_a_link_whose_video_is_being_made_again_says_preparing_not_gone( + state, db, as_user, make_user, _root, monkeypatch, upload_headers +): + # A link is made on a finished job; its asset can be (re)made under it later — a shared cache row + # re-fetched, a conversion retried. Until it is ready the page says "try again shortly". + _probe(monkeypatch, vcodec="h264") + c = as_user(make_user()) + job = c.post( + "/api/downloads/upload", content=b"h264 bytes", headers=upload_headers("clip.mp4") + ).json() + link = c.post(f"/api/downloads/{job['id']}/links", json={"allow_download": False}).json() + assert c.get(f"/api/public/watch/{link['token']}").status_code == 200 + db.get(MediaAsset, job["asset"]["id"]).status = state + db.commit() + r = c.get(f"/api/public/watch/{link['token']}") + assert r.status_code == 404 + assert r.json()["detail"]["reason"] == "video_preparing" + + +def test_the_admin_storage_view_counts_a_waiting_upload(db, as_user, make_user, _root, monkeypatch): + # The same disk the owner's footprint and the GC's cap count (`quota.ON_DISK`). + _probe(monkeypatch) + u = make_user() + _ingest(db, _root, u.id, size=10) + r = as_user(make_user(role="admin")).get("/api/admin/downloads/storage").json() + assert (r["ready_files"], r["total_bytes"]) == (1, 10) + assert {p["user_id"]: p["footprint_bytes"] for p in r["per_user"]} == {u.id: 10} + + +@pytest.mark.parametrize( + ("job_status", "asset_status", "ref_count"), + [ + ("done", "ready", 1), # a finished upload its owner keeps + ("queued", "pending", 1), # one waiting for the worker + ("canceled", "downloading", 0), # cancelled mid-conversion: the worker still reads it + ], +) +def test_the_upload_reap_leaves_one_still_held_or_still_read( + job_status, asset_status, ref_count, db, make_user, _root, monkeypatch +): + _probe(monkeypatch) + job, asset = _ingest(db, _root, make_user().id) + path = storage.abs_path(str(_root), asset.rel_path) + job.status, asset.status, asset.ref_count = job_status, asset_status, ref_count + db.commit() + + gcmod.run_download_gc(db) + + db.expire_all() + assert db.get(MediaAsset, asset.id) is not None + assert path.exists() diff --git a/backend/tests/test_worker_dispatch.py b/backend/tests/test_worker_dispatch.py new file mode 100644 index 00000000..8dc74b96 --- /dev/null +++ b/backend/tests/test_worker_dispatch.py @@ -0,0 +1,74 @@ +"""The worker child's dispatch (`runner._run`) and the edit path's last step before it stores a clip. + +`runner._run` picks the pipeline entry by the job's kind; the tests elsewhere call those entries +directly, so a kind routed to the wrong one (an upload sent to yt-dlp) would pass all of them. +""" + +from pathlib import Path + +import pytest + +from app.config import settings +from app.downloads import pipeline, runner, service, storage +from app.downloads import probe as probemod +from app.downloads import upload as uploadmod +from app.models import DownloadJob, MediaAsset + + +@pytest.mark.parametrize( + ("kind", "entry"), + [("upload", "_convert_upload"), ("edit", "_process_edit"), ("download", "_download")], +) +def test_each_job_kind_reaches_its_own_pipeline_entry(kind, entry, monkeypatch): + called = [] + for name in ("_convert_upload", "_process_edit", "_download"): + monkeypatch.setattr(pipeline, name, lambda *a, _n=name: called.append(_n)) + runner._run(5, 7, kind, "url", "https://example.org/v", {}, 1) + assert called == [entry] + + +def test_a_clip_the_phone_safe_step_changed_is_stored_under_its_new_extension( + db, make_user, tmp_path, monkeypatch +): + # A fast trim stream-copies its source's container; when the phone-safe step remuxes it (an + # audio clip out of an .mp4 becomes an .m4a), the stored path and container follow the new file. + monkeypatch.setattr(settings, "download_root", str(tmp_path)) + data = { + "streams": [{"codec_type": "audio", "codec_name": "aac"}], + "format": {"format_name": "mov,mp4", "duration": "10"}, + } + monkeypatch.setattr(probemod, "probe_json", lambda _p: data) + src = tmp_path / "in.bin" + src.write_bytes(b"a" * 10) + u = make_user() + source = uploadmod.ingest_upload(db, u.id, filename="song.m4a", src_path=src, size=10) + # Stored as an .mp4, the way an audio file was kept before every ready file met the rule. + kept = db.get(MediaAsset, source.asset_id) + old = storage.abs_path(str(tmp_path), kept.rel_path) + kept.rel_path, kept.container = kept.rel_path[: -len(".m4a")] + ".mp4", "mp4" + old.rename(storage.abs_path(str(tmp_path), kept.rel_path)) + db.commit() + job = service.enqueue_edit( + db, u.id, source, {"trim": {"start_s": 1, "end_s": 3}, "accurate": False} + ) + asset = db.get(MediaAsset, job.asset_id) + job.status, asset.status = "running", "downloading" + db.commit() + + def _trim(cmd, *_a, **_k): + Path(cmd[-1]).write_bytes(b"t" * 4) # the stream-copied clip, at the path ffmpeg was given + + def _remux(job_id, produced, out_dir, phase, total_s=None, **_k): + out = Path(out_dir) / "remuxed.m4a" + out.write_bytes(b"r" * 3) + return out + + monkeypatch.setattr(pipeline.editmod, "run_ffmpeg", _trim) + monkeypatch.setattr(pipeline, "_ensure_phone_safe", _remux) + pipeline._process_edit(job.id, asset.id) + + db.expire_all() + asset = db.get(MediaAsset, asset.id) + assert (db.get(DownloadJob, job.id).status, asset.container) == ("done", "m4a") + assert asset.rel_path.endswith(".m4a") + assert storage.abs_path(str(tmp_path), asset.rel_path).read_bytes() == b"rrr" diff --git a/frontend/src/components/CrashBoundary.tsx b/frontend/src/components/ui/CrashBoundary.tsx similarity index 100% rename from frontend/src/components/CrashBoundary.tsx rename to frontend/src/components/ui/CrashBoundary.tsx From dba4758c59bd1c3f1b46120ec296a66ada66ea45 Mon Sep 17 00:00:00 2001 From: npeter83 Date: Tue, 6 Oct 2026 03:19:32 +0200 Subject: [PATCH 07/12] fix(watch): a link whose video is being made again says so, in the page's language MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit The public watch route answers video_preparing for an asset that is pending or downloading, and the page shows "being prepared — try again" with a Retry instead of "this link is gone". watch.html ships one lang; the page now sets documentElement.lang to the language its strings were picked in. --- backend/app/routes/public.py | 4 ++ .../src/components/WatchPage.dom.test.tsx | 20 ++++++++++ frontend/src/components/WatchPage.tsx | 40 ++++++++++++++----- .../src/components/watchStrings.dom.test.ts | 30 ++++++++++++++ frontend/src/components/watchStrings.ts | 10 +++++ frontend/src/i18n/locales/en/errors.json | 1 + frontend/src/i18n/locales/hu/errors.json | 1 + 7 files changed, 96 insertions(+), 10 deletions(-) create mode 100644 frontend/src/components/watchStrings.dom.test.ts diff --git a/backend/app/routes/public.py b/backend/app/routes/public.py index 90bd0a4d..d8402e32 100644 --- a/backend/app/routes/public.py +++ b/backend/app/routes/public.py @@ -46,6 +46,10 @@ def _link_or_404(db: Session, token: str) -> DownloadLink: def _ready_target(db: Session, link: DownloadLink) -> tuple[DownloadJob, MediaAsset]: job = db.get(DownloadJob, link.job_id) asset = db.get(MediaAsset, job.asset_id) if job and job.asset_id else None + if asset is not None and asset.status in ("pending", "downloading"): + # Being (re)made — a re-download, or a conversion — so the link works again once it is ready: + # its own reason, so the page can say "try again shortly" rather than "this link is gone". + raise refused(404, "video_preparing") if job is None or asset is None or asset.status != "ready" or not asset.rel_path: raise refused(404, "video_gone") # Verify the file is actually on disk here (not just flagged ready), so the watch page never diff --git a/frontend/src/components/WatchPage.dom.test.tsx b/frontend/src/components/WatchPage.dom.test.tsx index 1395cab8..9bcab3db 100644 --- a/frontend/src/components/WatchPage.dom.test.tsx +++ b/frontend/src/components/WatchPage.dom.test.tsx @@ -211,6 +211,26 @@ describe("WatchPage honest failures", () => { expect(page.queryByRole("button", { name: "Try again" })).toBeNull(); }); + it("a 404 whose video is being made again says to come back, with a retry", async () => { + window.history.replaceState({}, "", "/watch/tok-preparing"); + vi.stubGlobal( + "fetch", + vi + .fn() + .mockResolvedValueOnce(res(404, { detail: { code: 404, reason: "video_preparing" } })) + .mockResolvedValue( + res(200, { needs_password: false, file_url: "/f.mp4", title: "A clip" }), + ), + ); + const page = render(); + const retry = await page.findByRole("button", { name: "Try again" }); + expect(page.getByRole("status").textContent).toContain("This video is being prepared."); + expect(page.queryByText(GONE)).toBeNull(); + + fireEvent.click(retry); + await waitFor(() => expect(page.container.querySelector("video")).not.toBeNull()); + }); + async function tryPassword(second: () => Promise) { vi.stubGlobal( "fetch", diff --git a/frontend/src/components/WatchPage.tsx b/frontend/src/components/WatchPage.tsx index 355cb66d..cb1a0023 100644 --- a/frontend/src/components/WatchPage.tsx +++ b/frontend/src/components/WatchPage.tsx @@ -76,13 +76,20 @@ function displayUrl(url: string): string { } } +/** What a 404 from the link means: `video_preparing` is a link whose video is being made again (a + * re-download, a conversion) and will work shortly; any other is a link that is gone. */ +async function notFoundState(r: Response): Promise<"preparing" | "gone"> { + const body = (await r.json().catch(() => null)) as { detail?: { reason?: string } } | null; + return body?.detail?.reason === "video_preparing" ? "preparing" : "gone"; +} + export default function WatchPage() { const token = window.location.pathname.split("/watch/")[1]?.split(/[/?#]/)[0] ?? ""; // Seeded from the URL rather than corrected by the effect below: with no token there is nothing // to load, and "loading" was committed and painted before the effect said so. - const [status, setStatus] = useState<"loading" | "ready" | "password" | "gone" | "error">( - token ? "loading" : "gone", - ); + const [status, setStatus] = useState< + "loading" | "ready" | "password" | "gone" | "preparing" | "error" + >(token ? "loading" : "gone"); const [meta, setMeta] = useState(null); const [password, setPassword] = useState(""); const [error, setError] = useState(null); @@ -110,16 +117,16 @@ export default function WatchPage() { useEffect(() => { if (!token) return; // the seed above already says "gone" send(`/api/public/watch/${encodeURIComponent(token)}`) - .then((r) => { - // Only a 404 says the link is gone (unknown, expired, revoked, or its file removed). Anything - // else — a 5xx while the server restarts, a dropped connection — is a failure worth retrying, - // not a dead link. - if (r.status === 404) return null; + .then((r): Promise => { + // Only a 404 says the link is gone (unknown, expired, revoked, or its file removed) — or that + // its video is being made again (`notFoundState`). Anything else — a 5xx while the server + // restarts, a dropped connection — is a failure worth retrying, not a dead link. + if (r.status === 404) return notFoundState(r); if (!r.ok) throw new Error(`HTTP ${r.status}`); return r.json() as Promise; }) .then((m) => { - if (!m) setStatus("gone"); + if (typeof m === "string") setStatus(m); else if (m.needs_password) setStatus("password"); else { setMeta(m); @@ -151,7 +158,7 @@ export default function WatchPage() { return; } if (r.status === 404) { - setStatus("gone"); + setStatus(await notFoundState(r)); return; } if (!r.ok) throw new Error(`HTTP ${r.status}`); @@ -194,6 +201,19 @@ export default function WatchPage() { )} + {status === "preparing" && ( +
+ {/* Announced like the failure below: it replaces the loading line. */} +
+
{T.preparing}
+
{T.preparingHint}
+ +
+
+ )} + {status === "error" && (
{/* Announced: it replaces the loading line, and this page has no live region of its own. */} diff --git a/frontend/src/components/watchStrings.dom.test.ts b/frontend/src/components/watchStrings.dom.test.ts new file mode 100644 index 00000000..9d470878 --- /dev/null +++ b/frontend/src/components/watchStrings.dom.test.ts @@ -0,0 +1,30 @@ +// @vitest-environment jsdom +// +// The shared-link page ships one watch.html for every visitor, so the document's language is set +// from the language its strings were picked in — a screen reader picks its voice by it, and the +// browser offers to translate by it. A Hungarian visitor once got lang="en" over Hungarian text. +import { afterEach, describe, expect, it, vi } from "vitest"; + +afterEach(() => { + vi.unstubAllGlobals(); + vi.resetModules(); +}); + +async function pageIn(language: string) { + vi.stubGlobal("navigator", { ...navigator, language }); + vi.resetModules(); + return import("./watchStrings"); +} + +describe("the watch page's document language", () => { + it.each([ + ["hu-HU", "hu"], + ["de-AT", "de"], + ["en-GB", "en"], + ["fr-FR", "en"], // no French strings: the page is in English, and says so + ])("a %s browser reads a page marked %s", async (language, want) => { + const { lang } = await pageIn(language); + expect(lang).toBe(want); + expect(document.documentElement.lang).toBe(want); + }); +}); diff --git a/frontend/src/components/watchStrings.ts b/frontend/src/components/watchStrings.ts index 18ab3c88..caca9def 100644 --- a/frontend/src/components/watchStrings.ts +++ b/frontend/src/components/watchStrings.ts @@ -18,6 +18,8 @@ const STR = { repeat: "Repeat", failed: "Couldn't load this link.", failedHint: "Check your connection and try again.", + preparing: "This video is being prepared.", + preparingHint: "Try again in a few minutes.", retry: "Try again", tooMany: "Too many attempts. Try again in a few minutes.", unlockFailed: "Couldn't check the password. Try again.", @@ -40,6 +42,8 @@ const STR = { repeat: "Ismétlés", failed: "Nem sikerült betölteni a linket.", failedHint: "Ellenőrizd a kapcsolatot, és próbáld újra.", + preparing: "A videó előkészítése folyamatban.", + preparingHint: "Próbáld újra néhány perc múlva.", retry: "Újrapróbálás", tooMany: "Túl sok próbálkozás. Próbáld újra pár perc múlva.", unlockFailed: "Nem sikerült ellenőrizni a jelszót. Próbáld újra.", @@ -62,6 +66,8 @@ const STR = { repeat: "Wiederholen", failed: "Der Link konnte nicht geladen werden.", failedHint: "Prüfe deine Verbindung und versuche es erneut.", + preparing: "Dieses Video wird gerade vorbereitet.", + preparingHint: "Versuche es in ein paar Minuten erneut.", retry: "Erneut versuchen", tooMany: "Zu viele Versuche. Versuche es in ein paar Minuten erneut.", unlockFailed: "Das Passwort konnte nicht geprüft werden. Versuche es erneut.", @@ -76,3 +82,7 @@ export const lang = ? ((navigator.language || "en").slice(0, 2) as keyof typeof STR) : "en"; export const T = STR[lang]; + +// The document says the language its text is in: a screen reader picks its voice by it, and the +// browser offers to translate by it. watch.html can only ship one; this page picks its own. +document.documentElement.lang = lang; diff --git a/frontend/src/i18n/locales/en/errors.json b/frontend/src/i18n/locales/en/errors.json index b01b9ff5..239658fb 100644 --- a/frontend/src/i18n/locales/en/errors.json +++ b/frontend/src/i18n/locales/en/errors.json @@ -191,6 +191,7 @@ "unknown_setting": "Unknown setting: {{key}}", "value_missing": "A value is required", "video_gone": "This video is no longer available", + "video_preparing": "This video is being prepared. Try again in a few minutes.", "view_name_taken": "You already have a view named “{{name}}”", "watch_sync_off": "Turn on Plex watch-sync first", "weak_password": "The password must be at least {{min}} characters", diff --git a/frontend/src/i18n/locales/hu/errors.json b/frontend/src/i18n/locales/hu/errors.json index b622dbc9..0b9125e7 100644 --- a/frontend/src/i18n/locales/hu/errors.json +++ b/frontend/src/i18n/locales/hu/errors.json @@ -191,6 +191,7 @@ "unknown_setting": "Ismeretlen beállítás: {{key}}", "value_missing": "Adj meg egy értéket", "video_gone": "Ez a videó már nem érhető el", + "video_preparing": "A videó előkészítése folyamatban. Próbáld újra néhány perc múlva.", "view_name_taken": "Már van „{{name}}” nevű nézeted", "watch_sync_off": "Előbb kapcsold be a Plex megtekintés-szinkront", "weak_password": "A jelszónak legalább {{min}} karakterből kell állnia", From 6073a006b8fb6232f34eaadb111a5cf42365cf29 Mon Sep 17 00:00:00 2001 From: npeter83 Date: Tue, 6 Oct 2026 03:19:32 +0200 Subject: [PATCH 08/12] fix(downloads): an upload's cancel, progress, failure and notice speak of an upload - Cancelling an upload asks "Delete upload?" and says the uploaded file is removed from the server with it. - The phone-safe conversion's real ffmpeg % shows as a bar ("Converting for phones"), not an indeterminate pulse. - A failed upload is titled "The upload isn't ready", with its own reasons (convert_failed, retryable; upload_missing) instead of download copy. - The inbox says "Upload ready to share" for a converted upload. --- .../DownloadCenter.confirm.dom.test.tsx | 17 ++++++++++++ .../NotificationsPanel.menu.dom.test.tsx | 16 +++++++++++ .../src/components/NotificationsPanel.tsx | 12 ++++++--- frontend/src/components/downloadModals.tsx | 6 ++--- .../src/components/downloadRow.dom.test.tsx | 24 +++++++++++++++++ frontend/src/components/downloadRow.tsx | 10 ++++--- .../src/components/useDownloadActions.tsx | 27 ++++++++++++++----- frontend/src/i18n/locales/en/downloads.json | 15 ++++++++++- frontend/src/i18n/locales/en/inbox.json | 3 +++ frontend/src/i18n/locales/hu/downloads.json | 15 ++++++++++- frontend/src/i18n/locales/hu/inbox.json | 3 +++ frontend/src/lib/api/downloads.ts | 2 +- frontend/src/lib/downloadFailure.test.ts | 21 ++++++++++++++- frontend/src/lib/downloadFailure.ts | 7 +++++ 14 files changed, 157 insertions(+), 21 deletions(-) diff --git a/frontend/src/components/DownloadCenter.confirm.dom.test.tsx b/frontend/src/components/DownloadCenter.confirm.dom.test.tsx index f4adee4c..46deb263 100644 --- a/frontend/src/components/DownloadCenter.confirm.dom.test.tsx +++ b/frontend/src/components/DownloadCenter.confirm.dom.test.tsx @@ -124,4 +124,21 @@ describe("DownloadCenter confirm dialogs", () => { ["downloads.confirm.deleteTitle", "downloads.actions.delete"], ]); }); + + it("cancelling an upload says the uploaded file goes with it", async () => { + // Nothing to fetch again: the cancel deletes the original from the server. + mocks.jobs = [{ ...job(3, "queued"), job_kind: "upload" }]; + render( + + + , + ); + await pick("Clip 3", "downloads.actions.cancel"); + await waitFor(() => expect(mocks.confirm).toHaveBeenCalledTimes(1)); + const [opts] = mocks.confirm.mock.calls[0]!; + expect([opts.title, opts.confirmLabel]).toEqual([ + "downloads.confirm.cancelUploadTitle", + "downloads.confirm.cancelUploadAction", + ]); + }); }); diff --git a/frontend/src/components/NotificationsPanel.menu.dom.test.tsx b/frontend/src/components/NotificationsPanel.menu.dom.test.tsx index d450ec12..5cb4ef94 100644 --- a/frontend/src/components/NotificationsPanel.menu.dom.test.tsx +++ b/frontend/src/components/NotificationsPanel.menu.dom.test.tsx @@ -364,3 +364,19 @@ describe("a download-lifecycle notice", () => { expect(screen.queryByText("download")).toBeNull(); }); }); + +describe("NotificationsPanel · a finished job's title", () => { + it("names what finished: a download, a clip, an upload", () => { + h.server = [ + server(1), + server(2, { data: { title: "Ready 2", kind: "edit" } }), + server(3, { data: { title: "Ready 3", kind: "upload" } }), + ]; + h.client = []; + mount(); + const titleOf = (text: string) => rowOf(text).textContent ?? ""; + expect(titleOf("Ready 1")).toContain("inbox.downloadDone.title"); + expect(titleOf("Ready 2")).toContain("inbox.editDone.title"); + expect(titleOf("Ready 3")).toContain("inbox.uploadDone.title"); + }); +}); diff --git a/frontend/src/components/NotificationsPanel.tsx b/frontend/src/components/NotificationsPanel.tsx index 9ea0cab7..134a5cfb 100644 --- a/frontend/src/components/NotificationsPanel.tsx +++ b/frontend/src/components/NotificationsPanel.tsx @@ -446,6 +446,11 @@ const DOWNLOAD_LIFECYCLE: Record = { download_expired: "downloadExpired", download_evicted: "downloadEvicted", }; +// A `download_done` notice's title by the producer's `kind`; any other kind reads as a download. +const DONE_TITLE: Record = { + edit: "inbox.editDone.title", + upload: "inbox.uploadDone.title", +}; function serverKind(n: AppNotification): "scheduler" | "download" | null { if (n.type === "scheduler" && typeof n.data?.job_id === "string") return "scheduler"; @@ -552,10 +557,11 @@ function NotificationRow({ title = t(`inbox.${lifecycle}.title`, { from: d.from ?? "" }); body = d.title || null; } else if (isDownloadDone) { - // "Download ready" / "Edit ready" per the producer's kind, with the video's title as the body - // (shipped in data.title). Same notification type; the copy differs so an edit isn't mislabelled. + // "Download ready" / "Edit ready" / "Upload ready" per the producer's kind, with the video's title + // as the body (shipped in data.title). Same notification type; the copy differs so an edit or an + // upload isn't called a download. const d = n.data as { title?: string | null; kind?: string }; - title = t(d.kind === "edit" ? "inbox.editDone.title" : "inbox.downloadDone.title"); + title = t(DONE_TITLE[d.kind ?? ""] ?? "inbox.downloadDone.title"); body = d.title || n.body; } return ( diff --git a/frontend/src/components/downloadModals.tsx b/frontend/src/components/downloadModals.tsx index 9de5d747..c7bb47ab 100644 --- a/frontend/src/components/downloadModals.tsx +++ b/frontend/src/components/downloadModals.tsx @@ -7,7 +7,7 @@ import Modal, { ModalFooter } from "./ui/Modal"; import { api, type DownloadJob } from "../lib/api"; import { Button } from "./ui/Button"; import { IconBtn } from "./downloadRow"; -import { failCopyKeys } from "../lib/downloadFailure"; +import { failCopyKeys, failTitleKey } from "../lib/downloadFailure"; import { Input } from "./ui/Input"; // Per-item user modals split out of DownloadCenter (self-contained, prop-driven): rename/metadata @@ -112,7 +112,7 @@ export function MetaEditModal({ job, onClose }: { job: DownloadJob; onClose: () // retry control regardless. `unknown` is deliberately NOT here: an unclassified failure is most often an // auth wall we couldn't parse (a login-walled Facebook post reads as a bare "no format"), so a // prominent Retry would mislead; force-retrying from the row stays available for genuine transients. -const RETRYABLE_ERRORS = ["postprocess", "network"]; +const RETRYABLE_ERRORS = ["postprocess", "network", "convert_failed"]; // A readable "why did this fail" window for a failed job: a category headline + plain explanation, // the raw yt-dlp/ffmpeg text behind a disclosure, and a Retry when the failure may be transient. @@ -130,7 +130,7 @@ export function ErrorModal({ const [showRaw, setShowRaw] = useState(false); const retryable = RETRYABLE_ERRORS.includes(code); return ( - +
{t(failCopyKeys(job, "short"))}

{t(failCopyKeys(job, "body"))}

diff --git a/frontend/src/components/downloadRow.dom.test.tsx b/frontend/src/components/downloadRow.dom.test.tsx index 7ef2ba2e..39df6c0c 100644 --- a/frontend/src/components/downloadRow.dom.test.tsx +++ b/frontend/src/components/downloadRow.dom.test.tsx @@ -292,3 +292,27 @@ describe("DownloadRow · no picture", () => { expect(container.querySelector("svg.lucide-film")).not.toBeNull(); }); }); + +// The phone-safe conversion reports ffmpeg's real progress, so it gets the % bar a download has; +// a yt-dlp post-step reports none and keeps the pulse. +describe("DownloadRow · a running conversion", () => { + const running = (phase: string): DownloadJob => ({ + ...baseJob, + status: "running", + progress: 40, + phase, + }); + + it("shows its phase and its %", () => { + const { container } = render(); + expect(screen.getByText("downloads.phase.converting")).toBeTruthy(); + expect(screen.getByText("40%")).toBeTruthy(); + expect(container.querySelector(".animate-pulse")).toBeNull(); + }); + + it("a post-step with no % keeps the pulse", () => { + const { container } = render(); + expect(screen.queryByText("40%")).toBeNull(); + expect(container.querySelector(".animate-pulse")).not.toBeNull(); + }); +}); diff --git a/frontend/src/components/downloadRow.tsx b/frontend/src/components/downloadRow.tsx index 9809a525..926e9c3d 100644 --- a/frontend/src/components/downloadRow.tsx +++ b/frontend/src/components/downloadRow.tsx @@ -14,7 +14,7 @@ import { relativeTime, } from "../lib/format"; import { navItemAttrs } from "../lib/listNav"; -import { failCopyKeys } from "../lib/downloadFailure"; +import { failCopyKeys, failTitleKey } from "../lib/downloadFailure"; import { displayUrl, hasOwnTitle, jobTitle } from "./downloadTitle"; import { IconButton } from "./ui/IconButton"; import { Pressable } from "./ui/Pressable"; @@ -34,12 +34,14 @@ const STATUS_CLS: Record = { }; // Byte-progress phases (show a % bar) vs ffmpeg post-steps (no %, indeterminate pulse). -// "editing" (the phase-2 editor) reports a real % from ffmpeg's out_time, so it gets a bar. -const DOWNLOAD_PHASES = ["video", "audio", "editing"]; +// "editing" (the phase-2 editor) and "converting" (the phone-safe conversion of a download or an +// upload) report a real % from ffmpeg's out_time, so they get a bar. +const DOWNLOAD_PHASES = ["video", "audio", "editing", "converting"]; const PHASE_KEYS = [ "video", "audio", "editing", + "converting", "merging", "audio_extract", "thumbnail", @@ -313,7 +315,7 @@ export function DownloadRow({ onShowError?.(job)} className="text-danger inline-block leading-6 min-h-6 touch-target pointer-coarse:leading-(--touch-target)" - title={t("downloads.errors.fail.title")} + title={t(failTitleKey(job))} > · {t(failCopyKeys(job, "short"))}{" "} diff --git a/frontend/src/components/useDownloadActions.tsx b/frontend/src/components/useDownloadActions.tsx index aa549678..1731c101 100644 --- a/frontend/src/components/useDownloadActions.tsx +++ b/frontend/src/components/useDownloadActions.tsx @@ -35,6 +35,18 @@ import { menuGroups } from "./ui/menuGroups"; export type DownloadTab = "queue" | "done" | "shared"; export type JobOp = "pause" | "resume" | "cancel" | "delete" | "redownload"; +// The Cancel confirmation of a queued job; an upload's names what it deletes. +const CANCEL_DOWNLOAD = { + title: "downloads.confirm.cancelTitle", + body: "downloads.confirm.cancelBody", + action: "downloads.confirm.cancelAction", +}; +const CANCEL_UPLOAD = { + title: "downloads.confirm.cancelUploadTitle", + body: "downloads.confirm.cancelUploadBody", + action: "downloads.confirm.cancelUploadAction", +}; + export function useDownloadActions({ onPlay, onEdit, @@ -147,13 +159,14 @@ export function useDownloadActions({ label: t("downloads.actions.cancel"), icon: icon(Ban), danger: true, - onSelect: () => - void confirmThen( - t("downloads.confirm.cancelTitle"), - t("downloads.confirm.cancelBody"), - t("downloads.confirm.cancelAction"), - () => act.mutate({ id: job.id, op: "cancel" }), - ), + // Cancelling an upload deletes the file it uploaded (there is nothing to fetch again), so + // its confirmation says so — "stop the download" would read as safe to undo. + onSelect: () => { + const copy = job.job_kind === "upload" ? CANCEL_UPLOAD : CANCEL_DOWNLOAD; + void confirmThen(t(copy.title), t(copy.body), t(copy.action), () => + act.mutate({ id: job.id, op: "cancel" }), + ); + }, }, ], ); diff --git a/frontend/src/i18n/locales/en/downloads.json b/frontend/src/i18n/locales/en/downloads.json index 95684f65..3130bb73 100644 --- a/frontend/src/i18n/locales/en/downloads.json +++ b/frontend/src/i18n/locales/en/downloads.json @@ -63,7 +63,8 @@ "sponsorblock": "Removing sponsors", "metadata": "Writing metadata", "processing": "Processing", - "editing": "Editing" + "editing": "Editing", + "converting": "Converting for phones" }, "actions": { "pause": "Pause", @@ -151,6 +152,9 @@ "cancelTitle": "Cancel download?", "cancelBody": "This stops the download and removes it from your queue.", "cancelAction": "Stop download", + "cancelUploadTitle": "Delete upload?", + "cancelUploadBody": "The file you uploaded is removed from the server along with it.", + "cancelUploadAction": "Delete upload", "deleteTitle": "Delete download?", "deleteBody": "This deletes it from your downloads. The file may stay cached for other users until it expires.", "removeSharedTitle": "Remove from your list?", @@ -226,6 +230,7 @@ }, "fail": { "title": "Couldn't download", + "titleUpload": "The upload isn't ready", "detailsLink": "details", "detailsToggle": "Technical details", "cat": { @@ -257,6 +262,14 @@ "short": "Couldn't process the video", "body": "The video downloaded but a processing step (merge or convert) failed. A retry often fixes this." }, + "convert_failed": { + "short": "Couldn't convert the upload", + "body": "Your file couldn't be converted into the format every phone plays. It may be damaged, or in a format the server can't read. The technical details below may help." + }, + "upload_missing": { + "short": "Uploaded file missing", + "body": "The file you uploaded is no longer on the server. Upload it again." + }, "source_missing": { "short": "Source unavailable", "body": "The original download this clip is based on is no longer available." diff --git a/frontend/src/i18n/locales/en/inbox.json b/frontend/src/i18n/locales/en/inbox.json index c4eb9bf9..c615c6ba 100644 --- a/frontend/src/i18n/locales/en/inbox.json +++ b/frontend/src/i18n/locales/en/inbox.json @@ -22,6 +22,9 @@ "editDone": { "title": "Edit ready" }, + "uploadDone": { + "title": "Upload ready to share" + }, "maintenance": { "title": "Videos removed", "body_one": "{{count}} saved or playlisted video was removed because it's no longer available on YouTube.", diff --git a/frontend/src/i18n/locales/hu/downloads.json b/frontend/src/i18n/locales/hu/downloads.json index 8371a869..7dd76372 100644 --- a/frontend/src/i18n/locales/hu/downloads.json +++ b/frontend/src/i18n/locales/hu/downloads.json @@ -63,7 +63,8 @@ "sponsorblock": "Szponzorok eltávolítása", "metadata": "Metaadat írása", "processing": "Feldolgozás", - "editing": "Szerkesztés" + "editing": "Szerkesztés", + "converting": "Átalakítás telefonra" }, "actions": { "pause": "Szünet", @@ -151,6 +152,9 @@ "cancelTitle": "Megszakítod a letöltést?", "cancelBody": "Ez leállítja a letöltést és eltávolítja a sorból.", "cancelAction": "Letöltés leállítása", + "cancelUploadTitle": "Törlöd a feltöltést?", + "cancelUploadBody": "A feltöltött fájl is törlődik a szerverről.", + "cancelUploadAction": "Feltöltés törlése", "deleteTitle": "Törlöd a letöltést?", "deleteBody": "Ez törli a letöltéseid közül. A fájl a lejáratáig még a gyorsítótárban maradhat mások számára.", "removeSharedTitle": "Eltávolítod a listádból?", @@ -226,6 +230,7 @@ }, "fail": { "title": "Nem sikerült letölteni", + "titleUpload": "A feltöltés nem készült el", "detailsLink": "részletek", "detailsToggle": "Technikai részletek", "cat": { @@ -257,6 +262,14 @@ "short": "Nem sikerült feldolgozni a videót", "body": "A videó letöltődött, de egy feldolgozási lépés (összefűzés vagy konvertálás) meghiúsult. Újrapróbálásra gyakran megjavul." }, + "convert_failed": { + "short": "Nem sikerült átalakítani", + "body": "A fájlt nem sikerült olyan formátumra alakítani, amit minden telefon lejátszik. Lehet, hogy sérült, vagy a szerver nem ismeri a formátumát. Az alábbi technikai részletek segíthetnek." + }, + "upload_missing": { + "short": "Hiányzik a feltöltött fájl", + "body": "A feltöltött fájl már nincs a szerveren. Töltsd fel újra." + }, "source_missing": { "short": "A forrás nem elérhető", "body": "Az eredeti letöltés, amelyből ez a vágás készült, már nem érhető el." diff --git a/frontend/src/i18n/locales/hu/inbox.json b/frontend/src/i18n/locales/hu/inbox.json index 8f16fdba..204501d7 100644 --- a/frontend/src/i18n/locales/hu/inbox.json +++ b/frontend/src/i18n/locales/hu/inbox.json @@ -22,6 +22,9 @@ "editDone": { "title": "Vágás kész" }, + "uploadDone": { + "title": "A feltöltés megosztható" + }, "maintenance": { "title": "Videók eltávolítva", "body_one": "{{count}} mentett vagy lejátszási listás videó eltávolítva, mert már nem elérhető a YouTube-on.", diff --git a/frontend/src/lib/api/downloads.ts b/frontend/src/lib/api/downloads.ts index fcac578d..231b66a8 100644 --- a/frontend/src/lib/api/downloads.ts +++ b/frontend/src/lib/api/downloads.ts @@ -89,7 +89,7 @@ export interface DownloadJob { display_uploader: string | null; // user override of the channel name display_uploader_url: string | null; // user override of the channel link extra_links: string[]; // extra reference URLs (beyond source_url) - job_kind?: DownloadJobKind; // "download" (default) | "edit" (a per-user trim/crop derivative) + job_kind?: DownloadJobKind; // see DownloadJobKind; absent reads as "download" source_job_id?: number | null; // parent download an edit was cut from edit_spec?: EditSpec | null; can_download: boolean; diff --git a/frontend/src/lib/downloadFailure.test.ts b/frontend/src/lib/downloadFailure.test.ts index d4e0dcb9..579c333c 100644 --- a/frontend/src/lib/downloadFailure.test.ts +++ b/frontend/src/lib/downloadFailure.test.ts @@ -6,7 +6,7 @@ // must still say the category, not print a key. import { describe, expect, it } from "vitest"; import i18n from "../i18n"; -import { failCopyKeys } from "./downloadFailure"; +import { failCopyKeys, failTitleKey } from "./downloadFailure"; describe("failCopyKeys", () => { it("puts a catalog entry before its category, and is the category alone without one", () => { @@ -47,3 +47,22 @@ describe("failCopyKeys", () => { ); }); }); + +describe("failTitleKey", () => { + it("titles an upload's failure as a conversion, anything else as a download", async () => { + await i18n.changeLanguage("en"); + // the locale has the copy (a missing key would come back as the key itself) + expect(i18n.t(failTitleKey({ job_kind: "upload" }))).not.toMatch(/^downloads\./); + expect(failTitleKey({ job_kind: "upload" })).toBe("downloads.errors.fail.titleUpload"); + expect(failTitleKey({ job_kind: "edit" })).toBe("downloads.errors.fail.title"); + expect(failTitleKey({})).toBe("downloads.errors.fail.title"); + }); + + it("an upload's own failures read as an upload's", async () => { + await i18n.changeLanguage("en"); + for (const code of ["convert_failed", "upload_missing"]) { + const short = i18n.t(failCopyKeys({ error_code: code, error_detail: null }, "short")); + expect(short).not.toBe(i18n.t("downloads.errors.fail.cat.unknown.short")); + } + }); +}); diff --git a/frontend/src/lib/downloadFailure.ts b/frontend/src/lib/downloadFailure.ts index 2b55c329..612f4918 100644 --- a/frontend/src/lib/downloadFailure.ts +++ b/frontend/src/lib/downloadFailure.ts @@ -18,3 +18,10 @@ export function failCopyKeys( ...(code === "unknown" ? [] : [category("unknown")]), ]; } + +/** The title over a failed job's explanation: an upload's failure is a conversion, not a download. */ +export function failTitleKey(job: Pick): string { + return job.job_kind === "upload" + ? "downloads.errors.fail.titleUpload" + : "downloads.errors.fail.title"; +} From b20e9a39472760cf9caae1fd9e323ea9b0406181 Mon Sep 17 00:00:00 2001 From: npeter83 Date: Tue, 6 Oct 2026 03:19:33 +0200 Subject: [PATCH 09/12] fix(player): the stacked layout's minimum width no longer clips the wide one The 36rem floor guards the stacked layout's width/height feedback loop; the wide side-panel card has no such loop, and on a short wide window the floor made the video taller than the card, cutting off YouTube's control bar. The floor now applies to the stacked layout only; e2e covers a 1400x380 window. --- frontend/e2e/player.spec.ts | 14 +++++++++++++- .../src/components/PlayerModal.dom.test.tsx | 2 +- frontend/src/components/PlayerModal.tsx | 18 ++++++++++-------- 3 files changed, 24 insertions(+), 10 deletions(-) diff --git a/frontend/e2e/player.spec.ts b/frontend/e2e/player.spec.ts index 8a234fa9..4e4fcfdc 100644 --- a/frontend/e2e/player.spec.ts +++ b/frontend/e2e/player.spec.ts @@ -29,7 +29,8 @@ test.describe("player modal", () => { // The video's size, in the windows the owner meets: layout bugs no DOM test can see. Each viewport // is a real one — 1920×911 is a 1080p screen at 100% (the side-panel layout), 1097×520 the same -// screen at 175% or a small laptop at 100% (the stacked layout, on a short window). +// screen at 175% or a small laptop at 100% (the stacked layout, on a short window), 1400×380 a wide +// window made short by docked devtools (the side-panel layout, whose card does not scroll). test.describe("player modal sizing", () => { test.beforeEach(async ({ page }) => { await page.route(/youtube\.com/, (route) => route.abort()); @@ -41,6 +42,7 @@ test.describe("player modal sizing", () => { for (const [w, h] of [ [1920, 911], [1097, 520], + [1400, 380], ] as const) { test(`the video keeps a real size at ${w}×${h}, and maximised fills the window`, async ({ page, @@ -54,6 +56,16 @@ test.describe("player modal sizing", () => { const stage = await box(page, ".player-stage"); expect(stage.width).toBeGreaterThanOrEqual(Math.min(w * 0.5, 560)); expect(stage.height).toBeGreaterThan(stage.width * 0.5); + // …and all of it is in sight: its bottom edge is the YouTube control bar, and the side-panel + // card clips what overflows it (it does not scroll), the stacked card scrolls to it. + const overflow = await page + .locator(".player-stage") + .first() + .evaluate((el) => { + const card = el.parentElement!; + return el.getBoundingClientRect().bottom - card.getBoundingClientRect().bottom; + }); + expect(overflow).toBeLessThanOrEqual(0.5); // Maximise (F): the stage is fixed to the viewport. In the side-panel layout its `self-start` // once aligned the fixed box too, and it shrank to its content's height — 0 px, a thin line. diff --git a/frontend/src/components/PlayerModal.dom.test.tsx b/frontend/src/components/PlayerModal.dom.test.tsx index 21d3a03d..774e903b 100644 --- a/frontend/src/components/PlayerModal.dom.test.tsx +++ b/frontend/src/components/PlayerModal.dom.test.tsx @@ -512,7 +512,7 @@ describe("PlayerModal — the side panel on a wide window", () => { expect(screen.queryByRole("button", { name: "player.description" })).toBeNull(); // The row's width: the video at the window's height, plus the panel and the side strips. expect(card.parentElement!.style.maxWidth).toBe( - "calc(max(36rem, (100dvh - 3rem - 0px) * 16 / 9) + 20rem + 7rem)", + "calc((100dvh - 3rem - 0px) * 16 / 9 + 20rem + 7rem)", ); }); diff --git a/frontend/src/components/PlayerModal.tsx b/frontend/src/components/PlayerModal.tsx index bce2d2b1..b4e24f3c 100644 --- a/frontend/src/components/PlayerModal.tsx +++ b/frontend/src/components/PlayerModal.tsx @@ -104,7 +104,7 @@ function canScrollBy(el: HTMLElement | null, dir: -1 | 1): boolean { // a rem width, so it grows with the text size; the video takes what is left. const WIDE_PLAYER_PX = 1280; const SIDE_PANEL = "20rem"; -// The narrowest the video gets to make room for the chrome under it (see the card's maxWidth). +// The narrowest the stacked video gets to make room for the chrome under it (see the card's maxWidth). const MIN_VIDEO = "36rem"; // The meta line's items each lead with a "·" that parts them from what stands before: the channel name @@ -614,7 +614,7 @@ export default function PlayerModal({ }; // The card's width budget (its maxWidth, below): the video's, and the side strips' gutters. - const videoWidth = `max(${MIN_VIDEO}, (100dvh - 3rem - ${below}px) * 16 / 9)`; + const fitWidth = `(100dvh - 3rem - ${below}px) * 16 / 9`; const gutters = hasQueue ? " + 7rem" : ""; // Portaled to , like every other full-screen overlay here. A fixed, full-viewport modal @@ -641,14 +641,16 @@ export default function PlayerModal({ className={`relative flex w-full max-h-full ${hasQueue ? "sm:px-14" : ""}`} // The design width (63rem with the side strips, else 56rem), narrowed so the 16:9 video plus the // chrome under it fits the window's height less the backdrop's padding (`p-6` = 3rem). - // Wide: no design cap — the video is as wide as the window's height allows, plus the side panel. - // Never narrower than MIN_VIDEO: the chrome under the video grows as the card narrows (its text - // wraps), so on a short window fitting it was a loop that ran the card down to 2 px (a laptop - // at 100%, a 1080p screen at 175%). Past the floor the card scrolls instead. + // Wide: no design cap — the video is as wide as the window's height allows, plus the side panel, + // whose content sits beside the video (`below` is ~0), so the fit is exact and the card, which + // does not scroll there, always holds the whole video. + // Stacked: never narrower than MIN_VIDEO. The chrome under the video grows as the card narrows + // (its text wraps), so on a short window fitting it was a loop that ran the card down to 2 px + // (a laptop at 100%, a 1080p screen at 175%). Past the floor the stacked card scrolls instead. style={{ maxWidth: wide - ? `calc(${videoWidth} + ${SIDE_PANEL}${gutters})` - : `min(${hasQueue ? "63rem" : "56rem"}, calc(${videoWidth}${gutters}))`, + ? `calc(${fitWidth} + ${SIDE_PANEL}${gutters})` + : `min(${hasQueue ? "63rem" : "56rem"}, calc(max(${MIN_VIDEO}, ${fitWidth})${gutters}))`, }} > {hasQueue && ( From d44d8d6dda73f3fdb51ce1725445b434bb67b9ac Mon Sep 17 00:00:00 2001 From: npeter83 Date: Tue, 6 Oct 2026 03:19:33 +0200 Subject: [PATCH 10/12] fix(gate): knip sees the watch entry, the build must emit watch.html, and two hand copies are held MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit - The raw-source contract tests read files from disk (rawSources.testkit) instead of import.meta.glob, which knip counted as importing every src file — its unused-files lane could never name an orphan. knip now names src/watchMain.tsx as an entry (registered in docs/gates.md). - vite build fails when index.html or watch.html is not emitted. - The watch-entry import walk follows dynamic import() too. - audit_metrics.py's plan-code patterns are held equal to the guard's. - CrashBoundary is domain-free: it moves into components/ui. --- docs/gates.md | 1 + frontend/knip.jsonc | 2 ++ frontend/src/components/ErrorBoundary.tsx | 2 +- .../listNav.wiring.contract.test.ts | 3 +- frontend/src/components/ui/CrashBoundary.tsx | 6 ++-- frontend/src/components/ui/README.md | 2 +- .../src/components/ui/Toaster.hosts.test.ts | 7 ++-- .../ui/pageMenuScope.contract.test.ts | 12 ++++--- .../ui/pressableSelectable.contract.test.ts | 11 ++++--- .../src/components/ui/uiI18n.contract.test.ts | 7 ++-- .../src/lib/mutationFailures.contract.test.ts | 7 ++-- .../lib/navPosition.wiring.contract.test.ts | 10 +++--- .../src/lib/nextPage.wiring.contract.test.ts | 7 ++-- frontend/src/lib/planCodes.contract.test.ts | 14 ++++++++ frontend/src/lib/plexImage.contract.test.ts | 14 ++++---- frontend/src/lib/rawSources.testkit.ts | 33 +++++++++++++++++++ frontend/src/watchEntry.contract.test.ts | 25 +++++++++++--- frontend/src/watchMain.tsx | 2 +- frontend/vite.config.ts | 9 +++++ 19 files changed, 119 insertions(+), 55 deletions(-) create mode 100644 frontend/src/lib/rawSources.testkit.ts diff --git a/docs/gates.md b/docs/gates.md index fc88f61e..75537e98 100644 --- a/docs/gates.md +++ b/docs/gates.md @@ -112,6 +112,7 @@ project checks needs a row. | coverage | not in the gate; no threshold | Owner decision E32 D4: never enforced; "every regression test seen red" is the bar, and a percentage invites tests written to move it. Out of the lanes since E35: read once, by hand, it found 5 bugs; as a lane it was a number nobody acted on, and the slowest vitest run (the one that flaked). On demand: docs/development.md. | frontend 80% / backend 78% of lines (E32 S1) | | CVE gate | HIGH/CRITICAL with a fix available only | `--ignore-unfixed` + `--severity HIGH,CRITICAL`: a finding with no fix cannot be acted on by an upgrade; it shows in the scan output. A fixable one fails the publish, and the only way past it is `.trivyignore.yaml`. | 0 open (E32 S1) | | npm audit | `--omit=dev --audit-level=high` | What ships to the browser, at high or above. The dev toolchain's advisories were burned down by hand in E32 S1 (18 → 0). | 0 | +| knip | `entry` — one file added | Not a narrowing: knip's Vite plugin reads entries from index.html only, so the shared link's page (`watch.html` → `src/watchMain.tsx`) is named here, or the lane reports that whole entry unused. It adds files to the lane, never takes any out. | 1 entry | | React Compiler | the functions it skips, pinned | `components/compilerCoverage.contract.test.ts` — the list may only shrink. End state: only VirtualGrid (TanStack Virtual, an IncompatibleLibrary) remains. | 93 (E32 S1) → 29 (E32 S3) → 1, VirtualGrid (E32 S4) | ## Accepted findings diff --git a/frontend/knip.jsonc b/frontend/knip.jsonc index e5bb1921..11a391cb 100644 --- a/frontend/knip.jsonc +++ b/frontend/knip.jsonc @@ -1,5 +1,7 @@ { "$schema": "https://unpkg.com/knip@6/schema.json", + // The shared link's page (watch.html): the Vite plugin reads only index.html for entries. + "entry": ["src/watchMain.tsx"], "project": ["**/*.{ts,tsx,js}"], "playwright": { "config": ["playwright.config.ts"], diff --git a/frontend/src/components/ErrorBoundary.tsx b/frontend/src/components/ErrorBoundary.tsx index 727e808d..ede7e369 100644 --- a/frontend/src/components/ErrorBoundary.tsx +++ b/frontend/src/components/ErrorBoundary.tsx @@ -1,7 +1,7 @@ import type { ReactNode } from "react"; import i18n from "../i18n"; import { notify } from "../lib/notifications"; -import { CrashBoundary, type CrashCopy } from "./CrashBoundary"; +import { CrashBoundary, type CrashCopy } from "./ui/CrashBoundary"; const copy = (): CrashCopy => ({ title: i18n.t("errors.boundary.title"), diff --git a/frontend/src/components/listNav.wiring.contract.test.ts b/frontend/src/components/listNav.wiring.contract.test.ts index f4c002c1..c34899a0 100644 --- a/frontend/src/components/listNav.wiring.contract.test.ts +++ b/frontend/src/components/listNav.wiring.contract.test.ts @@ -1,4 +1,5 @@ import { describe, expect, it } from "vitest"; +import { readSources } from "../lib/rawSources.testkit"; // Every rail and every card of a card list is walkable by the keyboard (lib/listNav). Two // opt-in halves, both checked here, because either one missing silently leaves the keyboard out — @@ -79,7 +80,7 @@ export function inList(code: string, pos: number): boolean { const has = (tag: Tag, prop: string) => new RegExp(`\\s${prop}\\b`).test(tag.text); function offenders(): { hits: string[]; tags: number; exempted: Set } { - const files = import.meta.glob("./**/*.tsx", { query: "?raw", import: "default", eager: true }); + const files = readSources(__dirname, "./**/*.tsx"); const hits: string[] = []; const exempted = new Set(); let tags = 0; diff --git a/frontend/src/components/ui/CrashBoundary.tsx b/frontend/src/components/ui/CrashBoundary.tsx index f1958794..5576ea82 100644 --- a/frontend/src/components/ui/CrashBoundary.tsx +++ b/frontend/src/components/ui/CrashBoundary.tsx @@ -1,7 +1,7 @@ import { Component, type ErrorInfo, type ReactNode } from "react"; -import { errorAttrs, log } from "../lib/log"; -import { reloadingForNewBuild } from "../lib/staleBuild"; -import { Button } from "./ui/Button"; +import { errorAttrs, log } from "../../lib/log"; +import { reloadingForNewBuild } from "../../lib/staleBuild"; +import { Button } from "./Button"; export interface CrashCopy { title: string; diff --git a/frontend/src/components/ui/README.md b/frontend/src/components/ui/README.md index 533ba48f..417a6166 100644 --- a/frontend/src/components/ui/README.md +++ b/frontend/src/components/ui/README.md @@ -41,7 +41,7 @@ Counter-examples, deliberately NOT here: `PosterTile` (knows about Plex montage | actions & inputs | `Button`, `IconButton`, `Pressable` (`selectable` when its text is a title or a name), `Chip` + `chipStyles`, `DragHandle` + `OrderButtons` / `useOrderItems` (the non-drag reorder, WCAG 2.5.7) (one per button family, docs/DESIGN.md; a raw `
@@ -220,7 +234,13 @@ export default function WatchPage() {
{T.failed}
{T.failedHint}
-
diff --git a/frontend/src/components/downloadAdmin.tsx b/frontend/src/components/downloadAdmin.tsx index b7fb5f59..dcc59e4e 100644 --- a/frontend/src/components/downloadAdmin.tsx +++ b/frontend/src/components/downloadAdmin.tsx @@ -186,7 +186,7 @@ export function AdminSystem() { {s && (
{[ - [t("downloads.admin.readyFiles"), String(s.ready_files)], + [t("downloads.admin.filesOnDisk"), String(s.files_on_disk)], [t("downloads.admin.totalSize"), formatBytes(s.total_bytes, lang)], [ t("downloads.admin.cap"), diff --git a/frontend/src/components/useDownloadActions.tsx b/frontend/src/components/useDownloadActions.tsx index 1731c101..8f1ac72a 100644 --- a/frontend/src/components/useDownloadActions.tsx +++ b/frontend/src/components/useDownloadActions.tsx @@ -16,7 +16,7 @@ import { Trash2, } from "lucide-react"; import { qk } from "../lib/queryKeys"; -import { api, type DownloadJob } from "../lib/api"; +import { api, type DownloadJob, type DownloadJobKind } from "../lib/api"; import { notifyActionError } from "../lib/actionErrors"; import { invalidateDownloads, quickShareLink } from "../lib/downloads"; import { libraryRowActions, sharedRowActions } from "../lib/downloadActions"; @@ -46,6 +46,29 @@ const CANCEL_UPLOAD = { body: "downloads.confirm.cancelUploadBody", action: "downloads.confirm.cancelUploadAction", }; +const CANCEL_EDIT = { + title: "downloads.confirm.cancelEditTitle", + body: "downloads.confirm.cancelEditBody", + action: "downloads.confirm.cancelEditAction", +}; +// Cancelling an upload deletes the file it uploaded (there is nothing to fetch again), so its +// confirmation says so — "stop the download" would read as safe to undo. A clip is being cut, not +// downloaded. +const CANCEL_COPY: Record = { + download: CANCEL_DOWNLOAD, + upload: CANCEL_UPLOAD, + edit: CANCEL_EDIT, +}; +// Deleting a download may leave its shared cache file for other holders. An upload and a clip are +// their owner's alone (a clip's asset is keyed by its user), so deleting one removes the file for good. +const DELETE_COPY: Record = { + download: { title: "downloads.confirm.deleteTitle", body: "downloads.confirm.deleteBody" }, + upload: { + title: "downloads.confirm.deleteUploadTitle", + body: "downloads.confirm.deleteUploadBody", + }, + edit: { title: "downloads.confirm.deleteEditTitle", body: "downloads.confirm.deleteEditBody" }, +}; export function useDownloadActions({ onPlay, @@ -159,10 +182,8 @@ export function useDownloadActions({ label: t("downloads.actions.cancel"), icon: icon(Ban), danger: true, - // Cancelling an upload deletes the file it uploaded (there is nothing to fetch again), so - // its confirmation says so — "stop the download" would read as safe to undo. onSelect: () => { - const copy = job.job_kind === "upload" ? CANCEL_UPLOAD : CANCEL_DOWNLOAD; + const copy = CANCEL_COPY[job.job_kind ?? "download"]; void confirmThen(t(copy.title), t(copy.body), t(copy.action), () => act.mutate({ id: job.id, op: "cancel" }), ); @@ -233,13 +254,12 @@ export function useDownloadActions({ label: t("downloads.actions.delete"), icon: icon(Trash2), danger: true, - onSelect: () => - void confirmThen( - t("downloads.confirm.deleteTitle"), - t("downloads.confirm.deleteBody"), - t("downloads.actions.delete"), - () => act.mutate({ id: job.id, op: "delete" }), - ), + onSelect: () => { + const copy = DELETE_COPY[job.job_kind ?? "download"]; + void confirmThen(t(copy.title), t(copy.body), t("downloads.actions.delete"), () => + act.mutate({ id: job.id, op: "delete" }), + ); + }, }, ], ); diff --git a/frontend/src/i18n/locales/en/downloads.json b/frontend/src/i18n/locales/en/downloads.json index 3130bb73..93d65bf6 100644 --- a/frontend/src/i18n/locales/en/downloads.json +++ b/frontend/src/i18n/locales/en/downloads.json @@ -155,8 +155,15 @@ "cancelUploadTitle": "Delete upload?", "cancelUploadBody": "The file you uploaded is removed from the server along with it.", "cancelUploadAction": "Delete upload", + "cancelEditTitle": "Cancel clip?", + "cancelEditBody": "This stops making the clip and removes it from your queue.", + "cancelEditAction": "Stop making this clip", "deleteTitle": "Delete download?", "deleteBody": "This deletes it from your downloads. The file may stay cached for other users until it expires.", + "deleteUploadTitle": "Delete upload?", + "deleteUploadBody": "The uploaded file is removed from the server. This can't be undone.", + "deleteEditTitle": "Delete clip?", + "deleteEditBody": "The clip is removed from the server. This can't be undone.", "removeSharedTitle": "Remove from your list?", "removeSharedBody": "This removes it from your shared list only — it won't delete the owner's file.", "redownloadTitle": "Re-download this file?", @@ -187,7 +194,7 @@ "deleteConfirm": "Delete the format “{{name}}”?" }, "admin": { - "readyFiles": "Ready files", + "filesOnDisk": "Files on disk", "totalSize": "Total size", "cap": "Cache cap", "noCap": "no cap", @@ -231,8 +238,20 @@ "fail": { "title": "Couldn't download", "titleUpload": "The upload isn't ready", + "titleEdit": "The clip isn't ready", "detailsLink": "details", "detailsToggle": "Technical details", + "upload": { + "quota_exceeded": { + "body": "The phone-ready version of this upload would not fit in your storage quota. Your original is kept – delete something from your library, then try again." + } + }, + "edit": { + "unknown": { + "short": "Couldn't make the clip", + "body": "Something went wrong while making this clip. The technical details below may help." + } + }, "cat": { "unavailable": { "short": "Video unavailable", diff --git a/frontend/src/i18n/locales/hu/downloads.json b/frontend/src/i18n/locales/hu/downloads.json index 7dd76372..3b371083 100644 --- a/frontend/src/i18n/locales/hu/downloads.json +++ b/frontend/src/i18n/locales/hu/downloads.json @@ -155,8 +155,15 @@ "cancelUploadTitle": "Törlöd a feltöltést?", "cancelUploadBody": "A feltöltött fájl is törlődik a szerverről.", "cancelUploadAction": "Feltöltés törlése", + "cancelEditTitle": "Megszakítod a klipet?", + "cancelEditBody": "Ez leállítja a klip készítését és eltávolítja a sorból.", + "cancelEditAction": "Klip készítésének leállítása", "deleteTitle": "Törlöd a letöltést?", "deleteBody": "Ez törli a letöltéseid közül. A fájl a lejáratáig még a gyorsítótárban maradhat mások számára.", + "deleteUploadTitle": "Törlöd a feltöltést?", + "deleteUploadBody": "A feltöltött fájl törlődik a szerverről. Ez nem vonható vissza.", + "deleteEditTitle": "Törlöd a klipet?", + "deleteEditBody": "A klip törlődik a szerverről. Ez nem vonható vissza.", "removeSharedTitle": "Eltávolítod a listádból?", "removeSharedBody": "Csak a te megosztott listádból tűnik el — a tulajdonos fájlját nem törli.", "redownloadTitle": "Újraletöltöd ezt a fájlt?", @@ -187,7 +194,7 @@ "deleteConfirm": "Törlöd a(z) „{{name}}” formátumot?" }, "admin": { - "readyFiles": "Kész fájlok", + "filesOnDisk": "Fájlok a lemezen", "totalSize": "Teljes méret", "cap": "Gyorsítótár-korlát", "noCap": "nincs korlát", @@ -231,8 +238,20 @@ "fail": { "title": "Nem sikerült letölteni", "titleUpload": "A feltöltés nem készült el", + "titleEdit": "A klip nem készült el", "detailsLink": "részletek", "detailsToggle": "Technikai részletek", + "upload": { + "quota_exceeded": { + "body": "A feltöltés telefonra alakított változata nem fér bele a tárhelykeretedbe. Az eredeti megmaradt – törölj valamit a könyvtáradból, majd próbáld újra." + } + }, + "edit": { + "unknown": { + "short": "Nem sikerült elkészíteni a klipet", + "body": "Hiba történt a klip készítése közben. Az alábbi technikai részletek segíthetnek." + } + }, "cat": { "unavailable": { "short": "A videó nem elérhető", diff --git a/frontend/src/lib/api/downloads.ts b/frontend/src/lib/api/downloads.ts index 231b66a8..33266730 100644 --- a/frontend/src/lib/api/downloads.ts +++ b/frontend/src/lib/api/downloads.ts @@ -47,7 +47,7 @@ export type DownloadStatus = "queued" | "running" | "paused" | "done" | "error" // "download" (default), "edit" (a per-user trim/crop derivative cut from another job), or "upload" // (a locally-uploaded file — no re-fetchable source, so re-download is rejected backend-side). // Local (not exported): consumers compare `job.job_kind` against literals, never name the type. -type DownloadJobKind = "download" | "edit" | "upload"; +export type DownloadJobKind = "download" | "edit" | "upload"; // Phase-2 editor recipe. A single `trim` (one output file) or a `segments` cut-list joined into // one file. `accurate` re-encodes for a frame-accurate cut (a crop always re-encodes). @@ -137,7 +137,7 @@ export interface DownloadUsage { } export interface AdminStorage { - ready_files: number; + files_on_disk: number; total_bytes: number; total_assets: number; total_cap_bytes: number; diff --git a/frontend/src/lib/downloadFailure.test.ts b/frontend/src/lib/downloadFailure.test.ts index 579c333c..8eba0bc3 100644 --- a/frontend/src/lib/downloadFailure.test.ts +++ b/frontend/src/lib/downloadFailure.test.ts @@ -49,13 +49,61 @@ describe("failCopyKeys", () => { }); describe("failTitleKey", () => { - it("titles an upload's failure as a conversion, anything else as a download", async () => { + it("titles a failure by what the job was making", async () => { await i18n.changeLanguage("en"); - // the locale has the copy (a missing key would come back as the key itself) - expect(i18n.t(failTitleKey({ job_kind: "upload" }))).not.toMatch(/^downloads\./); expect(failTitleKey({ job_kind: "upload" })).toBe("downloads.errors.fail.titleUpload"); - expect(failTitleKey({ job_kind: "edit" })).toBe("downloads.errors.fail.title"); + expect(failTitleKey({ job_kind: "edit" })).toBe("downloads.errors.fail.titleEdit"); + expect(failTitleKey({ job_kind: "download" })).toBe("downloads.errors.fail.title"); expect(failTitleKey({})).toBe("downloads.errors.fail.title"); + for (const lng of ["en", "hu"]) { + await i18n.changeLanguage(lng); + // the locale has the copy (a missing key would come back as the key itself) + for (const job_kind of ["upload", "edit"] as const) + expect(i18n.t(failTitleKey({ job_kind }))).not.toMatch(/^downloads\./); + } + }); + + it("an upload's quota refusal is worded as its conversion's, in both languages", async () => { + // The converted file outgrew the cap; the original stays. Not "this download was stopped because + // your quota is already used up" — the download sentence stays for a download. + const upload = { + error_code: "quota_exceeded", + error_detail: null, + job_kind: "upload", + } as const; + const download = { ...upload, job_kind: "download" } as const; + for (const lng of ["en", "hu"]) { + await i18n.changeLanguage(lng); + const body = i18n.t(failCopyKeys(upload, "body")); + expect(body).not.toMatch(/^downloads\./); + expect(body).not.toBe(i18n.t(failCopyKeys(download, "body"))); + expect(i18n.t(failCopyKeys(download, "body"))).toBe( + i18n.t("downloads.errors.fail.cat.quota_exceeded.body"), + ); + } + }); + + it("a clip that fails for no known reason says so of a clip, in both languages", async () => { + // Not "Couldn't download / Something went wrong with this download" — nothing was downloaded. + const clip = { error_code: "unknown", error_detail: null, job_kind: "edit" } as const; + const download = { ...clip, job_kind: "download" } as const; + for (const lng of ["en", "hu"]) { + await i18n.changeLanguage(lng); + for (const part of ["short", "body"] as const) { + const own = i18n.t(failCopyKeys(clip, part)); + expect(own).not.toMatch(/^downloads\./); + expect(own).not.toBe(i18n.t(failCopyKeys(download, part))); + } + } + // a category with no copy of its own falls back to the clip's unknown sentence, not the download's + expect( + failCopyKeys({ error_code: "brand_new", error_detail: null, job_kind: "edit" }, "body"), + ).toEqual([ + "downloads.errors.fail.edit.brand_new.body", + "downloads.errors.fail.cat.brand_new.body", + "downloads.errors.fail.edit.unknown.body", + "downloads.errors.fail.cat.unknown.body", + ]); }); it("an upload's own failures read as an upload's", async () => { diff --git a/frontend/src/lib/downloadFailure.ts b/frontend/src/lib/downloadFailure.ts index 612f4918..72f3a4d5 100644 --- a/frontend/src/lib/downloadFailure.ts +++ b/frontend/src/lib/downloadFailure.ts @@ -1,27 +1,38 @@ -import type { DownloadJob } from "./api"; +import type { DownloadJob, DownloadJobKind } from "./api"; // The i18n keys that explain a failed download, most specific first — the list form `t()` takes, so // the first key a locale has wins. A failure-catalog entry (`error_detail`, e.g. `facebook.login_wall`) // names what went wrong on that site and what to do; the category (`error_code`) is the generic // sentence, and the fallback when an older client meets a detail it has no copy for; a category it has -// no copy for falls back to the unknown one, so the row never prints a key. One helper for the queue -// row's headline and the failure window, so the two can never explain the same job differently. +// no copy for falls back to the unknown one, so the row never prints a key. A job that is not a +// download (an upload's conversion, a clip) may word a category its own way, ahead of the shared +// sentence — "this download was stopped" is not what happened to it — and its own unknown sentence +// comes before the download's. One helper for the queue row's headline and the failure window, so +// the two can never explain the same job differently. export function failCopyKeys( - job: Pick, + job: Pick, part: "short" | "body", ): string[] { - const category = (code: string) => `downloads.errors.fail.cat.${code}.${part}`; + const kind = job.job_kind ?? "download"; + const category = (code: string) => [ + ...(kind === "download" ? [] : [`downloads.errors.fail.${kind}.${code}.${part}`]), + `downloads.errors.fail.cat.${code}.${part}`, + ]; const code = job.error_code || "unknown"; return [ ...(job.error_detail ? [`downloads.errors.fail.catalog.${job.error_detail}.${part}`] : []), - category(code), - ...(code === "unknown" ? [] : [category("unknown")]), + ...category(code), + ...(code === "unknown" ? [] : category("unknown")), ]; } -/** The title over a failed job's explanation: an upload's failure is a conversion, not a download. */ +const TITLE_KEYS: Record = { + download: "downloads.errors.fail.title", + upload: "downloads.errors.fail.titleUpload", // its failure is a conversion, not a download + edit: "downloads.errors.fail.titleEdit", // a clip being cut +}; + +/** The title over a failed job's explanation, worded for what the job was making. */ export function failTitleKey(job: Pick): string { - return job.job_kind === "upload" - ? "downloads.errors.fail.titleUpload" - : "downloads.errors.fail.title"; + return TITLE_KEYS[job.job_kind ?? "download"]; } From c813814c3cd7ee60d56b129a05f0f9bb4faacae9 Mon Sep 17 00:00:00 2001 From: npeter83 Date: Tue, 6 Oct 2026 21:04:19 +0200 Subject: [PATCH 12/12] chore(release): 0.101.0 --- CHANGELOG.md | 11 +++++++++++ VERSION | 2 +- frontend/src/content/release-notes/en.json | 11 +++++++++++ frontend/src/content/release-notes/hu.json | 11 +++++++++++ 4 files changed, 34 insertions(+), 1 deletion(-) diff --git a/CHANGELOG.md b/CHANGELOG.md index ddbf2c08..47a6e808 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -7,6 +7,17 @@ edit the release notes and re-run the generator. Upgrading, pinning a version and rolling back: [docs/self-hosting.md](docs/self-hosting.md#upgrading). +## 0.101.0 — 2026-10-06 + +### New + +- A shared link plays on any phone, iPhone and iPad included: downloads, clips and uploads are kept in a format every phone plays, and an upload in another format is converted after it arrives. +- A shared link opens much faster: its page no longer loads the whole app. + +### Fixed + +- The YouTube player keeps its size on a short window and when maximised — before, it could shrink to a thin strip. Plus smaller fixes around uploads and clips. + ## 0.100.0 — 2026-10-05 ### New diff --git a/VERSION b/VERSION index 897e2158..0f00ab7c 100644 --- a/VERSION +++ b/VERSION @@ -1 +1 @@ -0.100.0 +0.101.0 diff --git a/frontend/src/content/release-notes/en.json b/frontend/src/content/release-notes/en.json index f9418aef..2f8b7faa 100644 --- a/frontend/src/content/release-notes/en.json +++ b/frontend/src/content/release-notes/en.json @@ -1,4 +1,15 @@ [ + { + "version": "0.101.0", + "date": "2026-10-06", + "new": [ + "A shared link plays on any phone, iPhone and iPad included: downloads, clips and uploads are kept in a format every phone plays, and an upload in another format is converted after it arrives.", + "A shared link opens much faster: its page no longer loads the whole app." + ], + "fixed": [ + "The YouTube player keeps its size on a short window and when maximised — before, it could shrink to a thin strip. Plus smaller fixes around uploads and clips." + ] + }, { "version": "0.100.0", "date": "2026-10-05", diff --git a/frontend/src/content/release-notes/hu.json b/frontend/src/content/release-notes/hu.json index f62c2b44..7a59bfa0 100644 --- a/frontend/src/content/release-notes/hu.json +++ b/frontend/src/content/release-notes/hu.json @@ -1,4 +1,15 @@ [ + { + "version": "0.101.0", + "date": "2026-10-06", + "new": [ + "A megosztott link minden telefonon lejátszható, iPhone-on és iPaden is: a letöltések, klipek és feltöltések olyan formátumban vannak, amelyet minden telefon lejátszik, a más formátumú feltöltés pedig beérkezés után átalakul.", + "A megosztott link sokkal gyorsabban nyílik meg: az oldala már nem tölti be az egész alkalmazást." + ], + "fixed": [ + "A YouTube-lejátszó alacsony ablakban és teljes méretre nagyítva is megtartja a méretét — eddig vékony csíkká zsugorodhatott. Továbbá kisebb javítások a feltöltések és klipek körül." + ] + }, { "version": "0.100.0", "date": "2026-10-05",