v3: the tests learn to expect trouble, and seven bugs they now catch are fixed — a review of the two test-suite commits found them written mostly for the happy path; each fix below comes with the test that fails without it. Infinite scroll: a later page that failed turned loadingMore back off, which made a new IntersectionObserver, which reports a sentinel still in view at once — so an offline feed asked again as fast as the request could fail, on Search, Index, the followed feeds, user dumps and user playlists alike. useInfiniteScroll now waits 1s, 2s, 4s… up to 30s while the sentinel stays put, counted from the last load so re-renders don't restart it, and asks at once again when the sentinel leaves the view or is pushed down by new items; the fake observer in src/test can report an element in view on observe(), as a browser's does. Playlist covers: the row was updated and playlist_updated sent before the file was written, so a client fetching image?v=<new> in between cached the old cover (or a 404) under the new URL for an hour, and a failed write left image_mime set with no file behind it. The file now goes in first, written beside and renamed over, and only then does the row change. Playlist order: the server wanted every member named, but GET hides private dumps the requester can't see, so a playlist holding someone else's dump that went private — or one a moderator reorders around the owner's private dumps — could never be reordered. The requester now orders what they can see and hidden members keep their slots; the visibility rule lives in api/lib/playlist-members.ts, shared by GET, reorder and the socket. playlist_dumps_updated also sent every member's id, private ones included, to a public playlist's whole audience; each viewer now gets the ids they can see, plus a dumpCount of all members so playlist cards keep the server's count. File dumps: replacing the file and changing visibility took two requests, so a failed second one left the new file live under the old visibility; PUT /api/dumps/:id/file takes isPrivate and applies both in one UPDATE, and it now broadcasts like any edit (it sent nothing, so other viewers never saw a new title). Notifications: one that arrived while the list was loading was dropped, then marked read by the read-all after the load, so it never showed; it's kept and merged in, once. Ranges: bytes=0-0,5-6 got a 416; several ranges, another unit or an unparseable header are ignored now and the whole file sent with a 200, as RFC 9110 allows — the old test asserted the 416.
All checks were successful
Build and Publish Docker Image / build-and-push (push) Successful in 49s

Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
This commit is contained in:
khannurien
2026-09-27 15:13:57 +00:00
parent 54e86c4366
commit c8bf0d583c
27 changed files with 672 additions and 85 deletions

View File

@@ -27,6 +27,7 @@ export interface PlaylistEvent {
playlist?: Playlist;
userId?: string;
dumpIds?: string[];
dumpCount?: number;
}
export interface CommentEvent {

View File

@@ -538,11 +538,12 @@ export function WSProvider({ children }: WSProviderProps) {
}
case "playlist_dumps_updated": {
const { playlistId, dumpIds } = msg;
const { playlistId, dumpIds, dumpCount } = msg;
setLastPlaylistEvent({
type: "dumps_updated",
playlistId,
dumpIds,
dumpCount,
});
break;
}

View File

@@ -1,4 +1,7 @@
import { useCallback, useEffect, useState } from "react";
import { useCallback, useEffect, useRef, useState } from "react";
/** Longest wait between two loads while the sentinel never left the view. */
const MAX_RETRY_DELAY_MS = 30_000;
/**
* Observe a sentinel element and call `onLoadMore` when it scrolls into view.
@@ -8,26 +11,69 @@ import { useCallback, useEffect, useState } from "react";
* changes — e.g. when the index page swaps the active feed tab and a fresh
* sentinel `<div>` mounts in place of the old one. A static ref would keep
* observing the now-detached node and silently stop loading more.
*
* Each change of `enabled` makes a new observer, which reports the sentinel
* at once if it's still in view. After a load that added items that's what
* fills a tall screen; after one that failed it would ask again straight
* away, and again, as fast as the request can fail. So a sentinel that stays
* put in view waits longer each time — 1s, 2s, 4s… up to 30s — and goes back
* to asking at once when it leaves the view or is pushed down by new items.
*/
export function useInfiniteScroll(onLoadMore: () => void, enabled: boolean) {
const [sentinel, setSentinel] = useState<HTMLDivElement | null>(null);
// Loads asked for since the sentinel last left the view or moved.
const attemptsRef = useRef(0);
const lastLoadAtRef = useRef(0);
const lastTopRef = useRef<number | null>(null);
const handleIntersect = useCallback(
(entries: IntersectionObserverEntry[]) => {
if (entries[0].isIntersecting && enabled) {
onLoadMore();
(entries: IntersectionObserverEntry[]): (() => void) | undefined => {
const entry = entries[0];
const top = entry.boundingClientRect?.top ?? null;
if (!entry.isIntersecting) {
attemptsRef.current = 0;
lastTopRef.current = null;
return;
}
if (
top !== null && lastTopRef.current !== null && top > lastTopRef.current
) {
attemptsRef.current = 0;
}
lastTopRef.current = top;
if (!enabled) return;
const attempts = attemptsRef.current;
const load = () => {
attemptsRef.current = attempts + 1;
lastLoadAtRef.current = Date.now();
onLoadMore();
};
// Counted from the last load, not from now: the observer is remade on
// every render that changes onLoadMore, which mustn't restart the wait.
const wait = attempts === 0 ? 0 : lastLoadAtRef.current +
Math.min(1000 * 2 ** (attempts - 1), MAX_RETRY_DELAY_MS) - Date.now();
if (wait <= 0) {
load();
return;
}
const timer = setTimeout(load, wait);
return () => clearTimeout(timer);
},
[onLoadMore, enabled],
);
useEffect(() => {
if (!sentinel) return;
const observer = new IntersectionObserver(handleIntersect, {
rootMargin: "200px",
});
let cancelPending: (() => void) | undefined;
const observer = new IntersectionObserver((entries) => {
cancelPending?.();
cancelPending = handleIntersect(entries);
}, { rootMargin: "200px" });
observer.observe(sentinel);
return () => observer.disconnect();
return () => {
cancelPending?.();
observer.disconnect();
};
}, [handleIntersect, sentinel]);
return setSentinel;

View File

@@ -87,7 +87,10 @@ export function usePlaylistListSync(
const idx = prev.findIndex((p) => p.id === ev.playlistId);
if (idx === -1) return prev;
const next = [...prev];
next[idx] = { ...next[idx], dumpCount: ev.dumpIds.length };
next[idx] = {
...next[idx],
dumpCount: ev.dumpCount ?? ev.dumpIds.length,
};
return next;
}

View File

@@ -527,7 +527,10 @@ export interface WSPlaylistDeletedMessage {
export interface WSPlaylistDumpsUpdatedMessage {
type: "playlist_dumps_updated";
playlistId: string;
/** The members this recipient can see, in order. */
dumpIds: string[];
/** Every member, as Playlist.dumpCount counts them. */
dumpCount: number;
}
export interface WSUserUpdatedMessage {
type: "user_updated";

View File

@@ -343,19 +343,13 @@ function DumpEditForm({
formData.append("title", trimmedTitle);
if (comment.trim()) formData.append("comment", comment.trim());
formData.append("categoryIds", JSON.stringify(categoryIds));
// In the same request, so the new file never goes out under the old
// visibility, and a failure leaves neither applied.
formData.append("isPrivate", String(isPrivate));
res = await authFetch(`${API_URL}/api/dumps/${dump.id}/file`, {
method: "PUT",
body: formData,
});
// Replacing the file doesn't touch visibility, so a change made in
// the same save goes separately, once the file is in.
if (isPrivate !== dump.isPrivate) {
await expectOk<RawDump>(res);
res = await authFetch(`${API_URL}/api/dumps/${dump.id}`, {
method: "PUT",
body: JSON.stringify({ isPrivate } satisfies UpdateDumpRequest),
});
}
} else {
// An emptied note is sent as null: left out, the server keeps the old
// one.

View File

@@ -30,7 +30,9 @@ import { friendlyFetchError } from "../utils/apiError.ts";
import { expectOk } from "../components/form/index.ts";
type State =
| { status: "loading" }
// `arrived`: sent over the socket before the list came back — the list may
// predate them, and the read-all that follows the load covers them anyway.
| { status: "loading"; arrived: Notification[] }
| { status: "error"; error: string }
| {
status: "loaded";
@@ -267,7 +269,10 @@ export function Notifications() {
useDocumentTitle(t`Notifications`);
const { authFetch } = useAuth();
const { clearUnreadNotifications, lastNotification } = useWS();
const [state, setState] = useState<State>({ status: "loading" });
const [state, setState] = useState<State>({
status: "loading",
arrived: [],
});
useEffect(() => {
let cancelled = false;
@@ -279,13 +284,19 @@ export function Notifications() {
),
);
if (cancelled) return;
setState({
const items = data.items.map(deserializeNotification);
setState((s) => ({
status: "loaded",
items: data.items.map(deserializeNotification),
items: [
...(s.status === "loading" ? s.arrived : []).filter((a) =>
!items.some((n) => n.id === a.id)
),
...items,
],
hasMore: data.hasMore,
page: 1,
loadingMore: false,
});
}));
} catch (err) {
if (!cancelled) {
setState({ status: "error", error: friendlyFetchError(err) });
@@ -319,6 +330,9 @@ export function Notifications() {
if (prevLastNotification !== lastNotification && lastNotification !== null) {
setPrevLastNotification(lastNotification);
setState((s) => {
if (s.status === "loading") {
return { ...s, arrived: [lastNotification, ...s.arrived] };
}
if (s.status !== "loaded") return s;
if (s.items.some((n) => n.id === lastNotification.id)) return s;
return { ...s, items: [lastNotification, ...s.items] };

View File

@@ -341,8 +341,8 @@ describe("dumps, playlists, comments, users", () => {
expect(ws().lastPlaylistEvent).toMatchObject({ type: "created", playlistId: "p" });
socket.receive({ type: "playlist_updated", playlist: wire(p) });
expect(ws().lastPlaylistEvent?.type).toBe("updated");
socket.receive({ type: "playlist_dumps_updated", playlistId: "p", dumpIds: ["x"] });
expect(ws().lastPlaylistEvent).toEqual({ type: "dumps_updated", playlistId: "p", dumpIds: ["x"] });
socket.receive({ type: "playlist_dumps_updated", playlistId: "p", dumpIds: ["x"], dumpCount: 2 });
expect(ws().lastPlaylistEvent).toEqual({ type: "dumps_updated", playlistId: "p", dumpIds: ["x"], dumpCount: 2 });
socket.receive({ type: "playlist_deleted", playlistId: "p", userId: "u" });
expect(ws().deletedPlaylistIds.has("p")).toBe(true);
expect(ws().lastPlaylistEvent).toEqual({ type: "deleted", playlistId: "p", userId: "u" });

View File

@@ -246,6 +246,87 @@ describe("useInfiniteScroll", () => {
intersect(true);
expect(onMore).toHaveBeenCalledTimes(1);
});
// A load that fails re-enables the feed with the sentinel still in view: a
// browser's new observer reports it at once, and without a pause that's a
// request loop as fast as the request can fail.
test("a sentinel that stays in view waits longer each time", () => {
vi.useFakeTimers();
const onMore = vi.fn();
render(<Feed onMore={onMore} enabled />);
intersect(true);
expect(onMore).toHaveBeenCalledTimes(1);
// Reported again right after each load, as a new observer does: how long
// until the next one?
const delays: number[] = [];
for (let i = 0; i < 8; i++) {
const before = onMore.mock.calls.length;
intersect(true);
let waited = 0;
while (onMore.mock.calls.length === before) {
act(() => vi.advanceTimersByTime(100));
waited += 100;
}
delays.push(waited);
}
expect(delays).toEqual([1000, 2000, 4000, 8000, 16000, 30000, 30000, 30000]);
});
test("re-rendering doesn't restart the wait", () => {
vi.useFakeTimers();
const onMore = vi.fn();
const { rerender } = render(<Feed onMore={onMore} enabled />);
intersect(true);
intersect(true);
act(() => vi.advanceTimersByTime(600));
// A new onLoadMore (the feed's state changed) remakes the observer.
rerender(<Feed onMore={() => onMore()} enabled />);
intersect(true);
act(() => vi.advanceTimersByTime(400));
expect(onMore).toHaveBeenCalledTimes(2);
});
test("leaving the view, or new items pushing it down, asks at once again", () => {
vi.useFakeTimers();
const onMore = vi.fn();
render(<Feed onMore={onMore} enabled />);
const at = (top: number) => {
const obs = FakeIntersectionObserver.instances.at(-1)!;
act(() =>
obs.callback(
[{ isIntersecting: true, boundingClientRect: { top } } as IntersectionObserverEntry],
obs as unknown as IntersectionObserver,
)
);
};
at(500);
at(500);
expect(onMore).toHaveBeenCalledTimes(1); // waiting
intersect(false);
intersect(true);
expect(onMore).toHaveBeenCalledTimes(2);
at(500);
at(900); // the page grew: that load worked
expect(onMore).toHaveBeenCalledTimes(3);
});
test("a pending retry is dropped when the feed is disabled or unmounted", () => {
vi.useFakeTimers();
const onMore = vi.fn();
const { rerender, unmount } = render(<Feed onMore={onMore} enabled />);
intersect(true);
intersect(true);
rerender(<Feed onMore={onMore} enabled={false} />);
act(() => vi.advanceTimersByTime(60_000));
expect(onMore).toHaveBeenCalledTimes(1);
rerender(<Feed onMore={onMore} enabled />);
intersect(true); // long overdue: at once
expect(onMore).toHaveBeenCalledTimes(2);
intersect(true);
unmount();
act(() => vi.advanceTimersByTime(60_000));
expect(onMore).toHaveBeenCalledTimes(2);
});
});
describe("useScrollLock", () => {

View File

@@ -186,6 +186,14 @@ describe("usePlaylistListSync", () => {
});
expect(hook.result.current.items[0].dumpCount).toBe(3);
});
test("dumps_updated counts the members this viewer can't see too", () => {
const { hook, emit } = setup([makePlaylist({ id: "a", dumpCount: 3 })]);
emit({
lastPlaylistEvent: { type: "dumps_updated", playlistId: "a", dumpIds: ["x", "y"], dumpCount: 4 },
});
expect(hook.result.current.items[0].dumpCount).toBe(4);
});
});
describe("usePositionAwareSync", () => {

View File

@@ -284,10 +284,9 @@ describe("DumpEdit page", () => {
expect(await screen.findByText("New blurb")).toBeInTheDocument();
});
test("replacing a file keeps a visibility change made alongside", async () => {
test("replacing a file sends a visibility change in the same request", async () => {
const fileDump = makeDump({ id: "f1", kind: "file", title: "Song", userId: "owner", fileName: "song.mp3", fileMime: "audio/mpeg", fileSize: 10, url: undefined, isPrivate: false });
route("PUT /api/dumps/:id/file", () => wire({ ...fileDump, fileName: "new.mp3" }));
route("PUT /api/dumps/:id", (req) => wire({ ...fileDump, ...(req.body as object) }));
route("PUT /api/dumps/:id/file", () => wire({ ...fileDump, fileName: "new.mp3", isPrivate: true }));
const user = openEdit(fileDump);
await screen.findByText("song.mp3");
const input = document.querySelector<HTMLInputElement>('.fdz input[type="file"]')!;
@@ -296,8 +295,25 @@ describe("DumpEdit page", () => {
await user.click(screen.getByRole("button", { name: "Save" }));
await screen.findByText("dump page");
const [put] = calls("PUT /api/dumps/:id/file");
expect((put.body as FormData).get("file")).toBeInstanceOf(File);
expect(calls("PUT /api/dumps/:id")[0]?.body).toMatchObject({ isPrivate: true });
const form = put.body as FormData;
expect(form.get("file")).toBeInstanceOf(File);
expect(form.get("isPrivate")).toBe("true");
expect(calls("PUT /api/dumps/:id")).toHaveLength(0);
});
test("a failed file replacement stays on the form, with nothing half-saved", async () => {
const fileDump = makeDump({ id: "f1", kind: "file", title: "Song", userId: "owner", fileName: "song.mp3", fileMime: "audio/mpeg", fileSize: 10, url: undefined, isPrivate: false });
route("PUT /api/dumps/:id/file", () => fail("File too large (max 50 MB)"));
const user = openEdit(fileDump);
await screen.findByText("song.mp3");
const input = document.querySelector<HTMLInputElement>('.fdz input[type="file"]')!;
await user.upload(input, new File([new Uint8Array(3)], "new.mp3", { type: "audio/mpeg" }));
await user.click(screen.getByRole("button", { name: "Private" }));
await user.click(screen.getByRole("button", { name: "Save" }));
expect(await screen.findByText("File too large (max 50 MB)")).toBeInTheDocument();
expect(screen.queryByText("dump page")).not.toBeInTheDocument();
expect(calls("PUT /api/dumps/:id/file")).toHaveLength(1);
expect(calls("PUT /api/dumps/:id")).toHaveLength(0);
});
test("deleting asks, then goes home", async () => {

View File

@@ -113,6 +113,27 @@ describe("Notifications", () => {
);
});
test("one that arrives while the list is loading isn't lost", async () => {
let answer!: (r: Response) => void;
route("GET /api/notifications", () => new Promise<Response>((r) => (answer = r)));
route("POST /api/notifications/read-all", () => ({}));
renderApp(<Notifications />, { session: me, route: "/notifications" });
const socket = FakeWebSocket.last;
socket.welcome({ unreadNotificationCount: 1 });
await waitFor(() => expect(calls("GET /api/notifications")).toHaveLength(1));
// Created after the list was read, so the list won't have it — but the
// read-all after the load will mark it read.
socket.receive({ type: "notification_created", notification: wire(n.upvoted) });
// And one the list does have, sent while it was on its way: shown once.
socket.receive({ type: "notification_created", notification: wire(n.follow) });
answer(page([n.follow]));
await screen.findByText(/started following you/);
const items = document.querySelectorAll(".notification-item");
expect(items).toHaveLength(2);
expect(items[0]).toHaveTextContent("bob upvoted Pic");
expect(items[0]).toHaveClass("notification-item--unread");
});
test("load more", async () => {
route("GET /api/notifications", (req) =>
req.url.searchParams.get("page") === "1"

View File

@@ -208,7 +208,7 @@ describe("PlaylistDetail", () => {
route("GET /api/dumps/:id", () => wire(makeDump({ id: "d", title: "Delta" })));
const { socket } = open({ session: visitor });
await screen.findByText("Alpha");
socket.receive({ type: "playlist_dumps_updated", playlistId: "p1", dumpIds: ["d", "a", "b", "c"] });
socket.receive({ type: "playlist_dumps_updated", playlistId: "p1", dumpIds: ["d", "a", "b", "c"], dumpCount: 4 });
expect(await screen.findByText("Delta")).toBeInTheDocument();
socket.receive({ type: "playlist_updated", playlist: wire(makePlaylist({ id: "p1", title: "Renamed", userId: "owner" })) });
expect(screen.getByRole("heading", { name: "Renamed" })).toBeInTheDocument();

View File

@@ -79,11 +79,29 @@ describe("Search", () => {
await act(() => new Promise((r) => setTimeout(r, 50)));
expect(screen.getByText("First")).toBeInTheDocument();
expect(screen.queryByRole("alert")).not.toBeInTheDocument();
// Scrolling away and back tries again at once.
const obs = FakeIntersectionObserver.instances.findLast((o) => o.elements.size > 0)!;
act(() => obs.callback([{ isIntersecting: false } as IntersectionObserverEntry], obs as never));
scroll();
await screen.findByText("Second");
expect(calls("GET /api/search").at(-1)!.url.searchParams.get("page")).toBe("2");
});
test("a later page that keeps failing isn't asked for in a loop", async () => {
route("GET /api/search", () => results({ dumps: [makeDump({ title: "First" })], hasMore: true }), { once: true });
route("GET /api/search", () => fail("down", 500));
// A short page: the sentinel is on screen and stays there.
FakeIntersectionObserver.inView = true;
open("/search?q=x");
await screen.findByText("First");
// In short steps: React holds what an act() schedules until it ends.
for (let i = 0; i < 20; i++) await act(() => new Promise((r) => setTimeout(r, 100)));
const pages = calls("GET /api/search").filter((c) => c.url.searchParams.get("page") !== "1");
// Once at once, once more a second later; the next is 2s after that.
expect(pages).toHaveLength(2);
expect(screen.getByText("First")).toBeInTheDocument();
});
test("a new query replaces the results", async () => {
route("GET /api/search", (req) =>
results({ dumps: [makeDump({ title: `About ${req.url.searchParams.get("q")}` })] }));

View File

@@ -17,6 +17,12 @@ i18n.activate("en");
class FakeIntersectionObserver {
static instances: FakeIntersectionObserver[] = [];
/**
* Whether observed elements are on screen. When set, a new observer reports
* its element as intersecting right after observe(), as a browser's does —
* which is what re-triggers a sentinel that never left the view.
*/
static inView = false;
elements = new Set<Element>();
callback: IntersectionObserverCallback;
constructor(callback: IntersectionObserverCallback) {
@@ -25,6 +31,14 @@ class FakeIntersectionObserver {
}
observe(el: Element) {
this.elements.add(el);
if (!FakeIntersectionObserver.inView) return;
setTimeout(() => {
if (!this.elements.has(el)) return;
this.callback(
[{ isIntersecting: true, target: el } as unknown as IntersectionObserverEntry],
this as unknown as IntersectionObserver,
);
});
}
unobserve(el: Element) {
this.elements.delete(el);
@@ -83,6 +97,7 @@ beforeEach(() => {
FakeWebSocket.reset();
vi.stubGlobal("WebSocket", FakeWebSocket);
FakeIntersectionObserver.instances = [];
FakeIntersectionObserver.inView = false;
});
afterEach(() => {