fix(files): review round 2, the page — the viewer's Escape, the dialog's drop, the capped list's honesty

- Escape closes the file viewer again: its layer sits above its dialog's, so it takes the Escape.
- The page's drop guard leaves a drag the open Upload dialog's zone accepted alone (it ran last and
  refused that drop).
- A capped list's server sort or search keeps the rows on screen until its answer is in (no skeleton
  under the pressed header), says "The first N items" when that answer is cut too, and an arrival is
  decided on the rows shown: one past the end of a cut list is said to be so, not gone.
- The search an arrival clears is taken at once.
- Tests: confirm names asserted per confirm, a fitting text pages under the arrows, a deleted file
  leaves the tray's finished list.
This commit is contained in:
2026-10-13 02:32:50 +02:00
parent 54a6bbf07f
commit 5bb101e493
6 changed files with 218 additions and 24 deletions
@@ -181,6 +181,30 @@ describe("FilePreview · stepping", () => {
expect(onFileChange).not.toHaveBeenCalled(); expect(onFileChange).not.toHaveBeenCalled();
}); });
it("a text whose lines fit pages under ← / →, even with the keyboard in it", async () => {
vi.spyOn(api, "previewText").mockResolvedValue({ text: "short", truncated: false });
const { onFileChange } = mount([file(1, "image"), file(8, "text"), file(9, "text")], 8);
await screen.findByText("short");
const region = screen.getByRole("region");
Object.defineProperty(region, "scrollWidth", { value: 300 });
Object.defineProperty(region, "clientWidth", { value: 300 });
fireEvent.keyDown(region, { key: "ArrowRight" });
expect(onFileChange).toHaveBeenLastCalledWith(9);
});
it("Escape closes it — after a layer opened above it has taken its own", () => {
const { onClose } = mount(list, 2);
const menu = pushLayer();
try {
fireEvent.keyDown(document, { key: "Escape" });
expect(onClose).not.toHaveBeenCalled();
} finally {
removeLayer(menu);
}
fireEvent.keyDown(document, { key: "Escape" });
expect(onClose).toHaveBeenCalledTimes(1);
});
it("stops at either end", () => { it("stops at either end", () => {
const { onFileChange } = mount(list, 1); const { onFileChange } = mount(list, 1);
fireEvent.keyDown(window, { key: "ArrowLeft" }); fireEvent.keyDown(window, { key: "ArrowLeft" });
+7 -5
View File
@@ -77,11 +77,13 @@ export default function FilePreview({
if (next) onFileChange(next.id); if (next) onFileChange(next.id);
}; };
useKeyScope("fileViewer"); useKeyScope("fileViewer");
// The viewer's own place on the layer stack (above its dialog): ← / → are its only while nothing is // The viewer's own place on the layer stack: ← / → are its only while nothing is on top of it — its
// on top of it — its "…" menu, a confirm, the Share or Details dialog opened from that menu. There an // "…" menu, a confirm, the Share or Details dialog opened from that menu. There an arrow moves
// arrow moves through the layer's own buttons; paging the viewer under it would show one file while // through the layer's own buttons; paging the viewer under it would show one file while the dialog
// the dialog acts on another. // acts on another. It mounts with its dialog (both only while there is a file), and effects run
const layer = useLayer(true, undefined, { passive: true }); // child first, so it sits just ABOVE the dialog's layer: the dialog is never topmost, and Escape is
// this layer's to take — passive, so the dialog keeps its Tab trap.
const layer = useLayer(!!file, onClose, { passive: true });
const onKey = useEffectEvent((e: KeyboardEvent) => { const onKey = useEffectEvent((e: KeyboardEvent) => {
if (layer.blockingAbove()) return; if (layer.blockingAbove()) return;
// The text, scrolled sideways by its arrows: a line wider than the stage is read that way. // The text, scrolled sideways by its arrows: a line wider than the stage is read that way.
+149 -3
View File
@@ -8,7 +8,15 @@
// list. i18n is key-passthrough, so labels read as keys. // list. i18n is key-passthrough, so labels read as keys.
import { useState } from "react"; import { useState } from "react";
import { afterEach, describe, expect, it, vi } from "vitest"; import { afterEach, describe, expect, it, vi } from "vitest";
import { act, cleanup, fireEvent, render, screen, waitFor } from "@testing-library/react"; import {
act,
cleanup,
createEvent,
fireEvent,
render,
screen,
waitFor,
} from "@testing-library/react";
import { QueryClient, QueryClientProvider } from "@tanstack/react-query"; import { QueryClient, QueryClientProvider } from "@tanstack/react-query";
import { api, type FileFolder, type LibraryFile } from "../lib/api"; import { api, type FileFolder, type LibraryFile } from "../lib/api";
import { formatDateTime } from "../lib/format"; import { formatDateTime } from "../lib/format";
@@ -28,6 +36,8 @@ const mocks = vi.hoisted(() => ({
fetching: false, fetching: false,
titleArgs: [] as unknown[], titleArgs: [] as unknown[],
searched: [] as unknown[], searched: [] as unknown[],
// The server's search/sort answer is still on its way (no data yet).
wideLoading: false,
foldersFailed: false, foldersFailed: false,
notify: vi.fn(), notify: vi.fn(),
})); }));
@@ -63,6 +73,14 @@ vi.mock("../lib/useLiveQuery", () => ({
if (opts?.enabled === false) return { data: undefined, isError: false, isLoading: false }; if (opts?.enabled === false) return { data: undefined, isError: false, isLoading: false };
mocks.keys.push(key); mocks.keys.push(key);
const asked = key[1] as { q?: string; sort?: string } | undefined; const asked = key[1] as { q?: string; sort?: string } | undefined;
if (key[0] === "files" && (asked?.q || asked?.sort) && mocks.wideLoading)
return {
data: undefined,
isError: false,
isLoading: true,
isFetching: true,
refetch: vi.fn(),
};
return { return {
data: data:
key[0] === "files" key[0] === "files"
@@ -87,7 +105,8 @@ vi.mock("./ui/PageShell", async (importOriginal) => ({
})); }));
import Files from "./Files"; import Files from "./Files";
import { FILES_LIST_CAP } from "../lib/api"; import { FILES_LIST_CAP, type QueueJob } from "../lib/api";
import { noteQueue, resetQueueWatch, useFinished } from "../lib/downloadQueue";
const file = (id: number, extra: Partial<LibraryFile> = {}): LibraryFile => const file = (id: number, extra: Partial<LibraryFile> = {}): LibraryFile =>
({ ({
@@ -133,6 +152,10 @@ afterEach(() => {
mocks.confirm.mockClear(); mocks.confirm.mockClear();
mocks.keys = []; mocks.keys = [];
mocks.fetching = false; mocks.fetching = false;
mocks.titleArgs = [];
mocks.wideLoading = false;
mocks.notify.mockClear();
resetQueueWatch();
}); });
function mount( function mount(
@@ -382,6 +405,26 @@ describe("Files · my file's row", () => {
fireEvent.click(menuItem("files.actions.delete")); fireEvent.click(menuItem("files.actions.delete"));
await waitFor(() => expect(del).toHaveBeenCalledWith(2)); await waitFor(() => expect(del).toHaveBeenCalledWith(2));
}); });
it("a deleted file leaves the tray's finished list: its Open would reach nothing", async () => {
mocks.files = [file(2)];
mocks.folders = [YT];
vi.spyOn(api, "deleteFile").mockResolvedValue({} as never);
const job = (status: string) => ({ id: 2, status }) as unknown as QueueJob;
noteQueue([job("running")]);
noteQueue([job("done")]);
let finished: number[] = [];
function Tray() {
finished = useFinished().map((j) => j.id);
return null;
}
render(<Tray />);
expect(finished).toEqual([2]);
mount();
await openMenu(() => fireEvent.contextMenu(rowOf("Clip 2")));
fireEvent.click(menuItem("files.actions.delete"));
await waitFor(() => expect(finished).toEqual([]));
});
}); });
describe("Files · a library past the list's cap", () => { describe("Files · a library past the list's cap", () => {
@@ -406,6 +449,83 @@ describe("Files · a library past the list's cap", () => {
expect(rowOf("Régi recept")).toBeTruthy(); expect(rowOf("Régi recept")).toBeTruthy();
expect(mocks.keys).toContainEqual(["files", { q: "recept" }]); expect(mocks.keys).toContainEqual(["files", { q: "recept" }]);
}); });
const capped = () => Array.from({ length: FILES_LIST_CAP }, (_, i) => file(i + 1));
const nameHeader = () => screen.getByRole("button", { name: /files\.columns\.name/ });
it("a sort asked of the server keeps the table on screen until its answer is in", () => {
mocks.folders = [YT];
mocks.files = capped();
mocks.wideLoading = true;
mount();
const header = nameHeader();
fireEvent.click(header);
// Not a skeleton in its place: that would take the header just pressed, and the keyboard with it.
expect(header.isConnected).toBe(true);
expect(rowOf("Clip 1")).toBeTruthy();
expect(screen.getByText("files.countNewest")).toBeTruthy();
});
it("says a list the server's sort cut holds only its first files; a short search is whole", () => {
mocks.folders = [YT];
mocks.files = capped();
mocks.searched = Array.from({ length: FILES_LIST_CAP }, (_, i) => file(i + 1000));
mount();
expect(screen.getByText("files.countNewest")).toBeTruthy();
fireEvent.click(nameHeader());
expect(screen.getByText("files.countFirst")).toBeTruthy();
cleanup();
mocks.searched = [file(9999, { display_name: "Régi recept" })];
mount(ALL_FILES, vi.fn(), "recept");
expect(screen.getByText("files.count")).toBeTruthy();
});
it("an arrival waits for the server's answer: the table can only turn to a file it holds", async () => {
mocks.folders = [YT];
mocks.files = capped();
mocks.wideLoading = true;
const consumeFocus = vi.fn();
mount(ALL_FILES, vi.fn(), "clip", { focus: { id: 3, token: 1 }, consumeFocus });
await new Promise((r) => setTimeout(r, 20));
expect(consumeFocus).not.toHaveBeenCalled();
});
it("a file past the end of a cut list is not said to be gone — it may only be older", async () => {
mocks.folders = [YT];
mocks.files = capped();
mount(ALL_FILES, vi.fn(), "", { focus: { id: 9999, token: 1 } });
await waitFor(() =>
expect(mocks.notify).toHaveBeenCalledWith({
level: "info",
message: "files.arrival.notListed",
}),
);
expect(mocks.notify).not.toHaveBeenCalledWith({ level: "info", message: "files.arrival.gone" });
});
it("an arrival's cleared search is asked of the server at once, not after the typing pause", () => {
mocks.folders = [YT];
mocks.files = capped();
mocks.searched = [];
const view = (search: string, focus: FilesFocus | null) => (
<QueryClientProvider client={new QueryClient()}>
<FilesActionsContext.Provider
value={{ setSearch: vi.fn(), setView: vi.fn(), openFile: vi.fn(), consumeFocus: vi.fn() }}
>
<FilesStateContext.Provider value={{ search, view: ALL_FILES, focus, arrived: null }}>
<Files />
</FilesStateContext.Provider>
</FilesActionsContext.Provider>
</QueryClientProvider>
);
const r = render(view("recept", null));
expect(mocks.keys).toContainEqual(["files", { q: "recept" }]);
mocks.keys = [];
// openFile: the search cleared and an arrival pending, in one step.
r.rerender(view("", { id: 3, token: 1 }));
expect(mocks.keys).not.toContainEqual(["files", { q: "recept" }]);
});
}); });
describe("Files · the confirms", () => { describe("Files · the confirms", () => {
@@ -429,6 +549,10 @@ describe("Files · the confirms", () => {
["downloads.confirm.redownloadTitle", "downloads.confirm.redownloadAction"], ["downloads.confirm.redownloadTitle", "downloads.confirm.redownloadAction"],
["downloads.confirm.deleteTitle", "downloads.actions.delete"], ["downloads.confirm.deleteTitle", "downloads.actions.delete"],
]); ]);
expect(mocks.titleArgs).toContainEqual([
"downloads.confirm.redownloadTitle",
{ name: "Clip 2" },
]);
mocks.confirm.mockResolvedValue(true); mocks.confirm.mockResolvedValue(true);
}); });
@@ -468,6 +592,10 @@ describe("Files · a shared file's row", () => {
]); ]);
fireEvent.click(menuItem("files.actions.removeShared")); fireEvent.click(menuItem("files.actions.removeShared"));
await waitFor(() => expect(remove).toHaveBeenCalledWith(4)); await waitFor(() => expect(remove).toHaveBeenCalledWith(4));
expect(mocks.titleArgs).toContainEqual([
"downloads.confirm.removeSharedTitle",
{ name: "Clip 4" },
]);
// The shared list is its own read, and nothing can be uploaded into it. // The shared list is its own read, and nothing can be uploaded into it.
expect(mocks.keys).toContainEqual(["files", { shared: true }]); expect(mocks.keys).toContainEqual(["files", { shared: true }]);
expect(screen.queryByText("files.actions.upload")).toBeNull(); expect(screen.queryByText("files.actions.upload")).toBeNull();
@@ -879,9 +1007,27 @@ describe("Files · dropping a file on the page", () => {
mount({ kind: "folder", id: 2 }); mount({ kind: "folder", id: 2 });
drag("drop", [new File(["x"], "recept.pdf")]); drag("drop", [new File(["x"], "recept.pdf")]);
expect(await screen.findByText("recept.pdf")).toBeTruthy(); expect(await screen.findByText("recept.pdf")).toBeTruthy();
expect(drag("dragover").defaultPrevented).toBe(true); const beside = drag("dragover");
expect(beside.defaultPrevented).toBe(true);
expect((beside as DragEvent).dataTransfer?.dropEffect).toBe("none");
expect(drag("drop", [new File(["y"], "masik.pdf")]).defaultPrevented).toBe(true); expect(drag("drop", [new File(["y"], "masik.pdf")]).defaultPrevented).toBe(true);
}); });
it("the open Upload dialog's own zone keeps the drop it accepted", async () => {
mocks.files = [];
mocks.folders = [YT, OWN];
vi.spyOn(api, "filesUsage").mockResolvedValue({ upload_max_bytes: 1_000_000 } as never);
mount({ kind: "folder", id: 2 });
drag("drop", [new File(["x"], "recept.pdf")]);
const zone = (await screen.findByText("recept.pdf")).closest("button")!;
const over = createEvent.dragOver(zone, {
dataTransfer: { types: ["Files"], files: [], dropEffect: "copy" },
}) as DragEvent;
fireEvent(zone, over);
// A "none" written by the page's own listener (it runs last) would refuse the zone's drop.
expect(over.defaultPrevented).toBe(true);
expect(over.dataTransfer?.dropEffect).toBe("copy");
});
}); });
describe("Files · when what it needs is missing", () => { describe("Files · when what it needs is missing", () => {
+30 -14
View File
@@ -202,7 +202,9 @@ export default function Files() {
// would miss an older file, so the server does both, over the whole view. Asked once the typing // would miss an older file, so the server does both, over the whole view. Asked once the typing
// pauses; a sort the server does not know (the source, the retention) stays on the rows. // pauses; a sort the server does not know (the source, the retention) stays on the rows.
const capped = (filesQ.data?.length ?? 0) >= FILES_LIST_CAP; const capped = (filesQ.data?.length ?? 0) >= FILES_LIST_CAP;
const typed = useDebounced(q, 300); // An arrival at one file clears the search (openFile): the cleared box is taken at once, not after
// a pause, so the file is looked for in the list it is about to be shown in.
const typed = useDebounced(q, 300, focus?.token);
const serverSort = sort && (FILE_SORTS as readonly string[]).includes(sort.key) ? sort : null; const serverSort = sort && (FILE_SORTS as readonly string[]).includes(sort.key) ? sort : null;
const wide = capped && (!!typed || !!serverSort); const wide = capped && (!!typed || !!serverSort);
const wideParams: FileListParams = { const wideParams: FileListParams = {
@@ -214,9 +216,14 @@ export default function Files() {
intervalMs: 15000, intervalMs: 15000,
enabled: wide, enabled: wide,
}); });
const listQ = wide ? wideQ : filesQ; // Until the server's first answer is in, the rows already on screen stay, searched and sorted here:
// a skeleton in their place would unmount the table under the header the user just pressed.
const wideIn = wide && (wideQ.data !== undefined || wideQ.isError);
const listQ = wideIn ? wideQ : filesQ;
const all = listQ.data ?? []; const all = listQ.data ?? [];
const rows = wide ? all : all.filter((f) => matches(f, q, lang)); const rows = wideIn ? all : all.filter((f) => matches(f, q, lang));
// The server's cap cut this list: the newest files of the view, or the first by its sort or search.
const cut = all.length >= FILES_LIST_CAP;
const listRef = useRef<HTMLDivElement>(null); const listRef = useRef<HTMLDivElement>(null);
const [renamingId, setRenamingId] = useState<number | null>(null); const [renamingId, setRenamingId] = useState<number | null>(null);
@@ -302,19 +309,23 @@ export default function Files() {
// (FilesProvider), which turns the table's or the grid's page to the file (`reveal`), puts the // (FilesProvider), which turns the table's or the grid's page to the file (`reveal`), puts the
// keyboard on it and rings it while this view is open. Not in this view, it is looked for among // keyboard on it and rings it while this view is open. Not in this view, it is looked for among
// every file; nowhere (gone since the link was made), it leaves nothing to arrive at, and is spent. // every file; nowhere (gone since the link was made), it leaves nothing to arrive at, and is spent.
const here = !!focus && !!filesQ.data?.some((f) => f.id === focus.id); // Looked for in the rows the page SHOWS — a server sort's own answer, not the rows standing in until
// it comes: the table can only turn to a file it holds.
const shownData = wide && !wideIn ? undefined : listQ.data;
const here = !!focus && !!shownData?.some((f) => f.id === focus.id);
// "Not here" only once the list is fresh: a just-finished upload is not in the cached rows yet. // "Not here" only once the list is fresh: a just-finished upload is not in the cached rows yet.
const missing = !!focus && !!filesQ.data && !here && !filesQ.isFetching; const missing = !!focus && !!shownData && !here && !listQ.isFetching;
const lookEverywhere = missing && shown.kind !== "all"; const lookEverywhere = missing && shown.kind !== "all";
useEffect(() => { useEffect(() => {
if (lookEverywhere) setView(ALL_FILES); if (lookEverywhere) setView(ALL_FILES);
else if (here) consumeFocus(viewKey); else if (here) consumeFocus(viewKey);
else if (missing) { else if (missing) {
consumeFocus(null); consumeFocus(null);
// Said, not swallowed: an old notice's file may have expired or been deleted since. // Said, not swallowed. A list the cap cut cannot tell a deleted file from one beyond its end;
notify({ level: "info", message: t("files.arrival.gone") }); // a whole list can: an old notice's file may have expired or been deleted since.
notify({ level: "info", message: t(cut ? "files.arrival.notListed" : "files.arrival.gone") });
} }
}, [here, missing, lookEverywhere, viewKey, setView, consumeFocus, t]); }, [here, missing, lookEverywhere, cut, viewKey, setView, consumeFocus, t]);
const arrival = arrived && arrived.view === viewKey ? arrived : null; const arrival = arrived && arrived.view === viewKey ? arrived : null;
const ringed = arrival?.id ?? null; const ringed = arrival?.id ?? null;
const reveal = arrival && { key: String(arrival.id), seq: arrival.token }; const reveal = arrival && { key: String(arrival.id), seq: arrival.token };
@@ -331,14 +342,16 @@ export default function Files() {
// ── A file dropped anywhere on the page ───────────────────────────────────────────────────────── // ── A file dropped anywhere on the page ─────────────────────────────────────────────────────────
// The page is the target, not a box on it: a short folder leaves most of the screen empty. Window // The page is the target, not a box on it: a short folder leaves most of the screen empty. Window
// listeners, since the drag starts outside the app. While the dialog is open its own zone takes the // listeners, since the drag starts outside the app. While the dialog is open its own zone takes the
// drop; a drag that carries no files (a link, selected text) is not ours. A file dropped where the // drop: a drag something under the pointer already accepted (that zone) is left as it set it — these
// page takes none (beside the open dialog, on the shared list) is still kept from the browser, // listeners run last, and a "none" written here would refuse the zone's drop. A drag that carries no
// which would open it in the tab — unloading the app and any upload under way. // files (a link, selected text) is not ours. A file dropped where the page takes none (beside the
// open dialog, on the shared list) is still kept from the browser, which would open it in the tab —
// unloading the app and any upload under way.
const [dragging, setDragging] = useState(false); const [dragging, setDragging] = useState(false);
const dragDepth = useRef(0); const dragDepth = useRef(0);
const dropReady = canUpload && upload === null; const dropReady = canUpload && upload === null;
const onDrag = useEffectEvent((e: DragEvent) => { const onDrag = useEffectEvent((e: DragEvent) => {
if (!e.dataTransfer?.types.includes("Files")) return; if (!e.dataTransfer?.types.includes("Files") || e.defaultPrevented) return;
e.preventDefault(); e.preventDefault();
if (!dropReady) { if (!dropReady) {
e.dataTransfer.dropEffect = "none"; e.dataTransfer.dropEffect = "none";
@@ -653,8 +666,11 @@ export default function Files() {
)} )}
{listQ.data && ( {listQ.data && (
<span className="text-xs tabular-nums text-muted"> <span className="text-xs tabular-nums text-muted">
{/* A capped list is the newest files of the view, and says so. */} {/* A list the cap cut says which files it holds: the newest, or the first by the
{t(capped && !wide ? "files.countNewest" : "files.count", { count: rows.length })} server's sort or search. */}
{t(cut ? (wideIn ? "files.countFirst" : "files.countNewest") : "files.count", {
count: rows.length,
})}
</span> </span>
)} )}
{isGrid && ( {isGrid && (
+4 -1
View File
@@ -5,10 +5,13 @@
"count_other": "{{count}} items", "count_other": "{{count}} items",
"countNewest_one": "The newest {{count}} item", "countNewest_one": "The newest {{count}} item",
"countNewest_other": "The newest {{count}} items", "countNewest_other": "The newest {{count}} items",
"countFirst_one": "The first {{count}} item",
"countFirst_other": "The first {{count}} items",
"rowMenu": "File actions", "rowMenu": "File actions",
"folderUnknown": "This folder could not be loaded", "folderUnknown": "This folder could not be loaded",
"arrival": { "arrival": {
"gone": "That file is no longer in your library" "gone": "That file is no longer in your library",
"notListed": "That file is past the end of this list — search for it by name"
}, },
"phoneOk": "Plays on a phone too", "phoneOk": "Plays on a phone too",
"rail": { "rail": {
+4 -1
View File
@@ -5,10 +5,13 @@
"count_other": "{{count}} elem", "count_other": "{{count}} elem",
"countNewest_one": "A legújabb {{count}} elem", "countNewest_one": "A legújabb {{count}} elem",
"countNewest_other": "A legújabb {{count}} elem", "countNewest_other": "A legújabb {{count}} elem",
"countFirst_one": "Az első {{count}} elem",
"countFirst_other": "Az első {{count}} elem",
"rowMenu": "Fájl műveletei", "rowMenu": "Fájl műveletei",
"folderUnknown": "Ez a mappa nem tölthető be", "folderUnknown": "Ez a mappa nem tölthető be",
"arrival": { "arrival": {
"gone": "Ez a fájl már nincs a könyvtáradban" "gone": "Ez a fájl már nincs a könyvtáradban",
"notListed": "Ez a fájl kimaradt a listából — keress rá a nevére"
}, },
"phoneOk": "Telefonon is lejátszható", "phoneOk": "Telefonon is lejátszható",
"rail": { "rail": {