Deep-review round 3, targeted at the surfaces round 2's fix commit changed. Five of the eight findings are the same shape: the guards written to stop half-done sweeps were themselves half-done. test_upload.py's display-field guard walked only `ast.Assign`, so the one writer with an unbounded REQUEST HEADER behind it — `spool_upload`'s `display_name=display_text(...)` keyword — was invisible to the guard meant to protect it. It also stopped at `app/routes/`, missing the four writers in `downloads/` that actually construct the row, and asserted `not offenders`, which enforces nothing once a refactor moves the writers out of its view. It now walks assignments AND keywords across `app/`, and holds exact set equality against a registry of all twelve write sites with each one's value. test_body_limit.py's release guard used `any(... ast.walk(fn))`, which accepts a `release_read_connection` that runs AFTER the await it exists to precede — satisfying the letter of the rule while pinning a connection for exactly the transfer it was supposed to free. Position is checked now, and a late release is its own, louder failure. The registry keyed sites by file basename while scanning recursively, and `admin.py`, `discover.py` and `playlists.py` each exist both at the top level and under `plex/` — the same entry-collapse the registry was widened to close. Keyed by relative path now. And all three guards recognised a body read only by the `capped_` naming convention, so a route spelling `await request.body()` inline declared no ceiling, released no connection, and was invisible to every one of them. `release_read_connection`'s precondition claimed the caller "must have NOTHING PENDING, and that is checked rather than described" while checking only UNFLUSHED ORM work. Measured: after `add`+`flush` all three collections are empty and the rollback loses the row, and Core DML never enters them at all — both of them this codebase's canonical write shapes. It now also asks the transaction directly via `pg_current_xact_id_if_assigned()`. `errors.tooLarge` was reworded to "try a smaller file", but that key answers every route's body cap: the issue composer's over-size JSON draft got advice about a file it does not have. Back to payload-neutral; the file-specific sentence keeps its own key. The upload dialog's new danger border outranked `dragOver`, so a user replacing an over-cap file got no sign the drop zone was live — and with a file picked the label shows its name rather than "drop it here", so the border was the only feedback left. Four further findings were refuted 3/3 and are not fixed: `aria-invalid` on the drop zone (measured in Chromium — the AX node does carry it on a native button), a bidi-override spoof through `display_text` (the served filename already strips those categories, and the proposed filter would have broken every ZWJ emoji), and `capped_body` not draining (the screenshot route's edge ceiling equals its own cap, so that refusal is unreachable, and the log sink is sent by beacon with nobody reading the answer).
181 lines
8.8 KiB
TypeScript
181 lines
8.8 KiB
TypeScript
// @vitest-environment jsdom
|
|
//
|
|
// E7·S5c — the local-upload dialog. It's a general file upload (any type, no format list): a picked
|
|
// file surfaces its name+size, and Upload streams it through api.uploadDownload (the XHR-progress
|
|
// path). This drives the real dialog against a mocked api so we can assert the file + optional name
|
|
// actually reach the ingest call, and that the success toast points at the library. i18n is
|
|
// key-passthrough.
|
|
import { describe, it, expect, vi, beforeEach, afterEach } from "vitest";
|
|
import { render, screen, fireEvent, waitFor, cleanup } from "@testing-library/react";
|
|
import { QueryClient, QueryClientProvider } from "@tanstack/react-query";
|
|
|
|
const h = vi.hoisted(() => ({
|
|
uploadDownload: vi.fn(async (_f: File, _o: unknown) => ({ id: 42 })),
|
|
// 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 })),
|
|
notify: vi.fn(),
|
|
}));
|
|
|
|
vi.mock("react-i18next", async (importOriginal) => {
|
|
const actual = await importOriginal<typeof import("react-i18next")>();
|
|
return { ...actual, useTranslation: () => ({ t: (key: string) => key }) };
|
|
});
|
|
vi.mock("../lib/api", () => ({
|
|
api: { uploadDownload: h.uploadDownload, downloadUsage: h.downloadUsage },
|
|
}));
|
|
vi.mock("../lib/downloads", () => ({ invalidateDownloads: vi.fn() }));
|
|
// The rest of the module stays real: the dialog is a ui/Modal, which hosts its own toasts.
|
|
vi.mock("../lib/notifications", async (importOriginal) => ({
|
|
...(await importOriginal<typeof import("../lib/notifications")>()),
|
|
notify: h.notify,
|
|
}));
|
|
vi.mock("../lib/nav", () => ({ navigateTo: vi.fn() }));
|
|
|
|
import UploadDialog from "./UploadDialog";
|
|
|
|
const wrap = (onClose = vi.fn()) => {
|
|
const qc = new QueryClient();
|
|
return render(
|
|
<QueryClientProvider client={qc}>
|
|
<UploadDialog onClose={onClose} />
|
|
</QueryClientProvider>,
|
|
);
|
|
};
|
|
|
|
// The dialog portals into document.body, so query the whole document, not the render container.
|
|
const fileInput = () => document.querySelector('input[type="file"]') as HTMLInputElement;
|
|
|
|
beforeEach(() => {
|
|
h.uploadDownload.mockClear();
|
|
h.notify.mockClear();
|
|
h.downloadUsage.mockClear();
|
|
h.downloadUsage.mockResolvedValue({ upload_max_bytes: 1_000_000 });
|
|
});
|
|
afterEach(cleanup);
|
|
|
|
describe("UploadDialog", () => {
|
|
it("shows the picked file's name and uploads it with the optional name", async () => {
|
|
const onClose = vi.fn();
|
|
wrap(onClose);
|
|
const f = new File(["hello world"], "notes.pdf", { type: "application/pdf" });
|
|
fireEvent.change(fileInput(), { target: { files: [f] } });
|
|
|
|
// The drop zone now surfaces the file name.
|
|
expect(screen.getByText("notes.pdf")).toBeTruthy();
|
|
|
|
// Optional display name.
|
|
const nameBox = screen.getByPlaceholderText("downloads.upload.namePlaceholder");
|
|
fireEvent.change(nameBox, { target: { value: "My notes" } });
|
|
|
|
fireEvent.click(screen.getByText("downloads.upload.submit"));
|
|
|
|
await waitFor(() => expect(h.uploadDownload).toHaveBeenCalledTimes(1));
|
|
const [passedFile, opts] = h.uploadDownload.mock.calls[0]!;
|
|
expect(passedFile).toBe(f);
|
|
expect((opts as { displayName?: string }).displayName).toBe("My notes");
|
|
await waitFor(() => expect(h.notify).toHaveBeenCalledTimes(1));
|
|
await waitFor(() => expect(onClose).toHaveBeenCalled());
|
|
});
|
|
|
|
it("keeps Upload disabled until a file is chosen", () => {
|
|
wrap();
|
|
const btn = screen.getByText("downloads.upload.submit").closest("button")!;
|
|
expect(btn.disabled).toBe(true);
|
|
});
|
|
|
|
// `file.size > cap`, on the boundary in both directions — the cap is inclusive server-side
|
|
// (backend test_upload.py pins that a body of EXACTLY the cap is accepted), so a dialog that
|
|
// refused it would make a legal file un-uploadable with no way round. Two sizes either side of
|
|
// one cap, because a single over-cap case passes just as happily with `>` loosened to `>=`.
|
|
const CAP = 8;
|
|
const pickFile = (bytes: number, name = "f.bin") =>
|
|
fireEvent.change(fileInput(), {
|
|
target: { files: [new File(["x".repeat(bytes)], name)] },
|
|
});
|
|
|
|
it("refuses a file one byte over the cap, naming the limit, instead of sending it", async () => {
|
|
// Both server-side caps answer only after the transfer — and on a default install the one that
|
|
// answers is the edge ceiling, whose refusal carries no number at all and may not even reach
|
|
// the browser. So the dialog checks the size itself: the user learns the limit before a byte
|
|
// moves.
|
|
h.downloadUsage.mockResolvedValue({ upload_max_bytes: CAP });
|
|
wrap();
|
|
await waitFor(() => expect(h.downloadUsage).toHaveBeenCalled());
|
|
pickFile(CAP + 1, "big.bin");
|
|
|
|
await waitFor(() => expect(screen.getByText("downloads.errors.upload.tooLarge")).toBeTruthy());
|
|
const btn = screen.getByText("downloads.upload.submit").closest("button")!;
|
|
expect(btn.disabled).toBe(true);
|
|
// The drop zone carries the reason, because a natively disabled Submit is not in the tab order
|
|
// to be asked for it.
|
|
const zone = screen.getByText("big.bin").closest("button")!;
|
|
expect(zone.getAttribute("aria-invalid")).toBe("true");
|
|
expect(zone.getAttribute("aria-describedby")).toBe(screen.getByRole("alert").id);
|
|
});
|
|
|
|
it("still highlights as a drop target while the picked file is over the cap", async () => {
|
|
// The state a user is in when they see the refusal is "replace this file". With a file
|
|
// already picked the zone's label shows its NAME rather than `dropActive`, so the border is
|
|
// the only feedback that a drag is over a live target — and a danger border that outranks the
|
|
// drag state reads as "drag-and-drop is off now", sending the user hunting for a control that
|
|
// does not exist. The drop would have worked.
|
|
h.downloadUsage.mockResolvedValue({ upload_max_bytes: CAP });
|
|
wrap();
|
|
await waitFor(() => expect(h.downloadUsage).toHaveBeenCalled());
|
|
pickFile(CAP + 1, "big.bin");
|
|
const zone = screen.getByText("big.bin").closest("button")!;
|
|
expect(zone.className).toContain("border-danger");
|
|
|
|
fireEvent.dragEnter(zone);
|
|
expect(zone.className).toContain("border-accent");
|
|
expect(zone.className).not.toContain("border-danger");
|
|
});
|
|
|
|
it("swallows a Submit press that arrives while the button still holds focus", async () => {
|
|
// The OTHER half of "never sent": the test above covers the natively disabled button (not
|
|
// focused), this one covers the soft-disabled one. `ui/Button` is focus-safe, so a button that
|
|
// turns unavailable while it HOLDS focus stays in the tab order and stays clickable, and it is
|
|
// the primitive's own click guard that refuses — measured: deleting the `tooBig` term from
|
|
// `submit()` changes nothing here, which is why that term is documented as belt and braces
|
|
// rather than as the mechanism. What this pins is the user-visible contract: pressing Upload
|
|
// after dropping a bigger file does not start a transfer the server will refuse.
|
|
h.downloadUsage.mockResolvedValue({ upload_max_bytes: CAP });
|
|
wrap();
|
|
await waitFor(() => expect(h.downloadUsage).toHaveBeenCalled());
|
|
pickFile(CAP, "fits.bin");
|
|
const btn = screen.getByText("downloads.upload.submit").closest("button")!;
|
|
btn.focus();
|
|
fireEvent.focus(btn);
|
|
pickFile(CAP + 1, "toobig.bin");
|
|
await waitFor(() => expect(screen.getByRole("alert")).toBeTruthy());
|
|
expect(btn.disabled).toBe(false); // soft-disabled, still focusable — and still clickable
|
|
fireEvent.click(btn);
|
|
expect(h.uploadDownload).not.toHaveBeenCalled();
|
|
});
|
|
|
|
it("sends a file of EXACTLY the cap, which the server accepts", async () => {
|
|
h.downloadUsage.mockResolvedValue({ upload_max_bytes: CAP });
|
|
wrap();
|
|
await waitFor(() => expect(h.downloadUsage).toHaveBeenCalled());
|
|
pickFile(CAP, "exact.bin");
|
|
expect(screen.queryByRole("alert")).toBeNull();
|
|
fireEvent.click(screen.getByText("downloads.upload.submit"));
|
|
await waitFor(() => expect(h.uploadDownload).toHaveBeenCalledTimes(1));
|
|
});
|
|
|
|
it("re-announces the refusal when a SECOND over-cap file is picked", async () => {
|
|
// Failing the same way again has to be announced again. The message names the CAP, not the
|
|
// file, so two over-cap picks leave the node and its text identical — and a live region that
|
|
// is never touched says nothing. See formAlerts.dom.test.tsx, which holds the same rule for
|
|
// the three other alerts in this app that can repeat.
|
|
h.downloadUsage.mockResolvedValue({ upload_max_bytes: CAP });
|
|
wrap();
|
|
await waitFor(() => expect(h.downloadUsage).toHaveBeenCalled());
|
|
pickFile(CAP + 1, "a.bin");
|
|
const first = await screen.findByRole("alert");
|
|
pickFile(CAP + 2, "b.bin");
|
|
await waitFor(() => expect(screen.getByRole("alert")).not.toBe(first));
|
|
});
|
|
});
|