fix(downloads): media-library UI review fixes (E7)
Deep-review remediation on the frontend, all findings adversarially verified: - LibraryPlayer picks audio vs video from the canonical media_kind (single source), not a hardcoded container whitelist, so an uploaded audio file with an uncommon/absent container renders in <audio> instead of a broken video box (legacy null media_kind keeps the heuristic). - UploadDialog aborts the in-flight upload when the dialog is closed by X/ESC/backdrop/Back (not only the footer Cancel); drag highlight uses an enter/leave depth counter to stop the flicker; the progress bar gets a stable role=progressbar aria-label with the value on aria-valuenow. - DownloadCenter's Shared tab renders honest loading/error states instead of a blank panel, and the library/shared row-action gating moves to a single tested helper (lib/downloadActions). Tests: downloadActions gating matrix, the uploadReq XHR path (abort/401/422-localize/onerror), and the media_kind-driven audio/video branch. i18n: downloads.upload.uploadProgress (HU + EN).
This commit is contained in:
@@ -35,6 +35,7 @@ import { useLiveQuery } from "../lib/useLiveQuery";
|
||||
import { useMe } from "../lib/useMe";
|
||||
import { api, type DownloadJob } from "../lib/api";
|
||||
import { invalidateDownloads, quickShareLink } from "../lib/downloads";
|
||||
import { libraryRowActions, sharedRowActions } from "../lib/downloadActions";
|
||||
import { inputCls } from "./ui/form";
|
||||
import { Button } from "./ui/Button";
|
||||
import { DownloadRow, IconBtn, UsageBar } from "./downloadRow";
|
||||
@@ -280,28 +281,22 @@ export default function DownloadCenter() {
|
||||
<UsageBar />
|
||||
<div className="space-y-2">
|
||||
{library.map((job) => {
|
||||
// E7·S5b — gate row actions on the stored media_kind. A generic uploaded file
|
||||
// (media_kind='file') is download+share only: no player and no timeline to trim
|
||||
// (Play/Edit are media-only), and no metadata/channel edit. Re-download needs a
|
||||
// re-fetchable source, which an edit clip and an upload both lack (backend rejects
|
||||
// both), so it's a true-download-only action.
|
||||
const isFile = job.asset?.media_kind === "file";
|
||||
const isMedia = !isFile;
|
||||
const canRedownload = job.job_kind !== "edit" && job.job_kind !== "upload";
|
||||
// Row actions gated on the stored media_kind + job_kind (see downloadActions).
|
||||
const a = libraryRowActions(job);
|
||||
return (
|
||||
<DownloadRow key={job.id} job={job}>
|
||||
{isMedia && (
|
||||
{a.play && (
|
||||
<IconBtn onClick={() => setPlayJob(job)} title={t("downloads.actions.play")}>
|
||||
<PlayCircle className="w-4 h-4" />
|
||||
</IconBtn>
|
||||
)}
|
||||
{saveBtn(job)}
|
||||
{isMedia && job.can_download && (job.asset?.duration_s ?? 0) > 0 && (
|
||||
{a.edit && (
|
||||
<IconBtn onClick={() => setEditJob(job)} title={t("downloads.actions.edit")}>
|
||||
<Scissors className="w-4 h-4" />
|
||||
</IconBtn>
|
||||
)}
|
||||
{isMedia && (
|
||||
{a.editDetails && (
|
||||
<IconBtn
|
||||
onClick={() => setMetaJob(job)}
|
||||
title={t("downloads.actions.editDetails")}
|
||||
@@ -318,7 +313,7 @@ export default function DownloadCenter() {
|
||||
>
|
||||
<Link2 className="w-4 h-4" />
|
||||
</IconBtn>
|
||||
{canRedownload && (
|
||||
{a.redownload && (
|
||||
<IconBtn
|
||||
onClick={() =>
|
||||
confirmThen(
|
||||
@@ -367,18 +362,17 @@ export default function DownloadCenter() {
|
||||
{tab === "shared" && (
|
||||
<div className="space-y-2">
|
||||
{(sharedQ.data ?? []).map((job) => {
|
||||
// Same media_kind gate as the library (S5b): a shared generic file is download-only —
|
||||
// no player, no trim. Its owner set can_download, which still gates save/edit here.
|
||||
const isMedia = job.asset?.media_kind !== "file";
|
||||
// Same media_kind + can_download gate as the library, via the shared helper (S5b).
|
||||
const a = sharedRowActions(job);
|
||||
return (
|
||||
<DownloadRow key={job.id} job={job}>
|
||||
{isMedia && job.can_download && (
|
||||
{a.play && (
|
||||
<IconBtn onClick={() => setPlayJob(job)} title={t("downloads.actions.play")}>
|
||||
<PlayCircle className="w-4 h-4" />
|
||||
</IconBtn>
|
||||
)}
|
||||
{job.can_download && saveBtn(job)}
|
||||
{isMedia && job.can_download && (job.asset?.duration_s ?? 0) > 0 && (
|
||||
{a.save && saveBtn(job)}
|
||||
{a.edit && (
|
||||
<IconBtn onClick={() => setEditJob(job)} title={t("downloads.actions.edit")}>
|
||||
<Scissors className="w-4 h-4" />
|
||||
</IconBtn>
|
||||
@@ -399,11 +393,18 @@ export default function DownloadCenter() {
|
||||
</DownloadRow>
|
||||
);
|
||||
})}
|
||||
{sharedQ.data && sharedQ.data.length === 0 && (
|
||||
<div className="text-sm text-muted py-10 text-center">
|
||||
{t("downloads.empty.shared")}
|
||||
</div>
|
||||
)}
|
||||
{(sharedQ.data ?? []).length === 0 &&
|
||||
(sharedQ.isError && !sharedQ.data ? (
|
||||
<StateMessage tone="error" onRetry={() => sharedQ.refetch()}>
|
||||
{t("common.loadError")}
|
||||
</StateMessage>
|
||||
) : sharedQ.isLoading ? (
|
||||
<LoadingState />
|
||||
) : (
|
||||
<div className="text-sm text-muted py-10 text-center">
|
||||
{t("downloads.empty.shared")}
|
||||
</div>
|
||||
))}
|
||||
</div>
|
||||
)}
|
||||
|
||||
|
||||
@@ -1,9 +1,9 @@
|
||||
// @vitest-environment jsdom
|
||||
//
|
||||
// E7·S4b — the in-app library player streams the completed item's LOCAL file into a native media
|
||||
// element (no transcode; S4a guarantees a browser-native container). A video asset (has pixel dims)
|
||||
// renders <video src=/file>; an audio-only asset (m4a/mp3, no dims) renders <audio src=/file>. i18n is
|
||||
// key-passthrough here.
|
||||
// element. video/audio is chosen from the stored media_kind (the single source), NOT a container
|
||||
// list — so an audio upload with an uncommon/absent container still renders <audio src=/file>. i18n
|
||||
// is key-passthrough here.
|
||||
import { describe, it, expect, afterEach, vi } from "vitest";
|
||||
import { render, cleanup } from "@testing-library/react";
|
||||
|
||||
@@ -89,11 +89,37 @@ describe("LibraryPlayer", () => {
|
||||
expect(document.body.querySelector("audio")).toBeNull();
|
||||
});
|
||||
|
||||
it("plays an audio-only asset (no dims, m4a) in an <audio> element", () => {
|
||||
render(<LibraryPlayer job={withAsset({ container: "m4a" })} onClose={() => {}} />);
|
||||
it("plays an audio-only asset (media_kind='audio') in an <audio> element", () => {
|
||||
render(
|
||||
<LibraryPlayer
|
||||
job={withAsset({ media_kind: "audio", container: "m4a" })}
|
||||
onClose={() => {}}
|
||||
/>,
|
||||
);
|
||||
const audio = document.body.querySelector("audio");
|
||||
expect(audio).not.toBeNull();
|
||||
expect(audio!.getAttribute("src")).toBe("/api/downloads/7/file");
|
||||
expect(document.body.querySelector("video")).toBeNull();
|
||||
});
|
||||
|
||||
it("uses media_kind — an audio upload with an uncommon container still renders <audio>", () => {
|
||||
// The exact bug the fix targets: the old container whitelist (m4a/mp3/…) would drop an .m4b/.weba
|
||||
// audio upload into a <video> box. media_kind='audio' is authoritative regardless of container.
|
||||
render(
|
||||
<LibraryPlayer
|
||||
job={withAsset({ media_kind: "audio", container: "m4b" })}
|
||||
onClose={() => {}}
|
||||
/>,
|
||||
);
|
||||
expect(document.body.querySelector("audio")).not.toBeNull();
|
||||
expect(document.body.querySelector("video")).toBeNull();
|
||||
});
|
||||
|
||||
it("falls back to the container heuristic for a legacy null media_kind", () => {
|
||||
// A pre-discriminator asset (media_kind null) still resolves audio vs video from dims+container.
|
||||
render(
|
||||
<LibraryPlayer job={withAsset({ media_kind: null, container: "mp3" })} onClose={() => {}} />,
|
||||
);
|
||||
expect(document.body.querySelector("audio")).not.toBeNull();
|
||||
});
|
||||
});
|
||||
|
||||
@@ -8,19 +8,24 @@ import { jobTitle } from "./downloadRow";
|
||||
// YouTube IFrame shell) or PlexPlayer (a Plex HLS/session shell): both are bound to their own domain
|
||||
// APIs and can't point at a local file. Instead we reuse the shared a11y shell (`Modal`: focus-trap,
|
||||
// ESC/back-to-close, scroll-lock, layer-stack, portal) and stream the local file into a NATIVE
|
||||
// <video>/<audio controls> — the same element VideoEditor already plays this URL in. S4a guarantees
|
||||
// every download is a browser-native container (mp4/H.264/AAC + m4a/mp3), so native playback (seek via
|
||||
// the range-aware /file endpoint, volume, fullscreen, speed) always works with zero transcode.
|
||||
// <video>/<audio controls> — the same element VideoEditor already plays this URL in. Downloads are a
|
||||
// browser-native container (mp4/H.264/AAC + m4a/mp3), so native playback (seek via the range-aware
|
||||
// /file endpoint, volume, fullscreen, speed) works with zero transcode; an uploaded media item may be
|
||||
// any codec (best-effort playback).
|
||||
export default function LibraryPlayer({ job, onClose }: { job: DownloadJob; onClose: () => void }) {
|
||||
const { t } = useTranslation();
|
||||
const fileUrl = useMemo(() => api.downloadFileUrl(job.id), [job.id]);
|
||||
// Video vs audio-only: a video asset carries real pixel dimensions; audio (m4a/mp3) has none. Fall
|
||||
// back to the container kind so a dimension-less-but-video asset still gets the <video> element.
|
||||
// Video vs audio-only comes from the stored media_kind (the single source every sibling reads —
|
||||
// WatchPage/downloadRow/DownloadCenter) — NOT a re-derived container list, which would misclassify
|
||||
// an uploaded audio file whose container is outside a hardcoded set (uploads accept any container).
|
||||
// Only fall back to the old dimension+container heuristic for a legacy asset with a null media_kind.
|
||||
const asset = job.asset;
|
||||
const isAudio =
|
||||
!(asset?.width && asset?.height) &&
|
||||
!!asset?.container &&
|
||||
["m4a", "mp3", "opus", "ogg", "wav", "flac", "aac"].includes(asset.container.toLowerCase());
|
||||
asset?.media_kind === "audio" ||
|
||||
(asset?.media_kind == null &&
|
||||
!(asset?.width && asset?.height) &&
|
||||
!!asset?.container &&
|
||||
["m4a", "mp3", "opus", "ogg", "wav", "flac", "aac"].includes(asset.container.toLowerCase()));
|
||||
// Our self-hosted poster (always same-origin, never expires) is the primary; the source thumbnail is
|
||||
// the fallback. Same precedence as the library-row thumbnail.
|
||||
const poster = job.poster_url ? api.posterImageUrl(job.id) : (asset?.thumbnail_url ?? undefined);
|
||||
|
||||
@@ -1,4 +1,4 @@
|
||||
import { useRef, useState } from "react";
|
||||
import { useEffect, useRef, useState } from "react";
|
||||
import { useTranslation } from "react-i18next";
|
||||
import { useQueryClient } from "@tanstack/react-query";
|
||||
import { UploadCloud, File as FileIcon } from "lucide-react";
|
||||
@@ -24,8 +24,16 @@ export default function UploadDialog({ onClose }: { onClose: () => void }) {
|
||||
const [pct, setPct] = useState<number | null>(null); // null = idle; 0–100 while uploading
|
||||
const inputRef = useRef<HTMLInputElement>(null);
|
||||
const abortRef = useRef<AbortController | null>(null);
|
||||
// dragenter/dragleave fire as the cursor crosses the drop zone's OWN children (icon/text), so a
|
||||
// plain boolean flickers. Count enter vs leave and only clear the highlight at depth 0.
|
||||
const dragDepth = useRef(0);
|
||||
const busy = pct !== null;
|
||||
|
||||
// Closing the dialog mid-upload (X / ESC / backdrop / browser Back all unmount us) must abort the
|
||||
// in-flight XHR — otherwise it keeps streaming in the background and still toasts success. Only the
|
||||
// footer Cancel wired the abort; this covers every other dismissal.
|
||||
useEffect(() => () => abortRef.current?.abort(), []);
|
||||
|
||||
const pick = () => inputRef.current?.click();
|
||||
|
||||
const submit = async () => {
|
||||
@@ -62,13 +70,19 @@ export default function UploadDialog({ onClose }: { onClose: () => void }) {
|
||||
<button
|
||||
type="button"
|
||||
onClick={pick}
|
||||
onDragOver={(e) => {
|
||||
onDragEnter={(e) => {
|
||||
e.preventDefault();
|
||||
dragDepth.current += 1;
|
||||
setDragOver(true);
|
||||
}}
|
||||
onDragLeave={() => setDragOver(false)}
|
||||
onDragOver={(e) => e.preventDefault()}
|
||||
onDragLeave={() => {
|
||||
dragDepth.current = Math.max(0, dragDepth.current - 1);
|
||||
if (dragDepth.current === 0) setDragOver(false);
|
||||
}}
|
||||
onDrop={(e) => {
|
||||
e.preventDefault();
|
||||
dragDepth.current = 0;
|
||||
setDragOver(false);
|
||||
const f = e.dataTransfer.files?.[0];
|
||||
if (f) {
|
||||
@@ -123,7 +137,16 @@ export default function UploadDialog({ onClose }: { onClose: () => void }) {
|
||||
|
||||
{busy && (
|
||||
<div className="mb-5" aria-live="polite">
|
||||
<div className="h-2 rounded-full bg-surface overflow-hidden">
|
||||
<div
|
||||
className="h-2 rounded-full bg-surface overflow-hidden"
|
||||
role="progressbar"
|
||||
aria-valuenow={pct ?? 0}
|
||||
aria-valuemin={0}
|
||||
aria-valuemax={100}
|
||||
// A STABLE descriptor as the name; the live percentage rides aria-valuenow (and the visible
|
||||
// <p> below re-announces it via the aria-live wrapper) — don't bake the value into the name.
|
||||
aria-label={t("downloads.upload.uploadProgress")}
|
||||
>
|
||||
<div
|
||||
className="h-full bg-accent transition-[width] duration-150"
|
||||
style={{ width: `${pct ?? 0}%` }}
|
||||
|
||||
Reference in New Issue
Block a user