From c8bf0d583cf39098f05ede4f2a0a8c53556e8e52 Mon Sep 17 00:00:00 2001 From: khannurien Date: Sun, 27 Sep 2026 15:13:57 +0000 Subject: [PATCH] =?UTF-8?q?v3:=20the=20tests=20learn=20to=20expect=20troub?= =?UTF-8?q?le,=20and=20seven=20bugs=20they=20now=20catch=20are=20fixed=20?= =?UTF-8?q?=E2=80=94=20a=20review=20of=20the=20two=20test-suite=20commits?= =?UTF-8?q?=20found=20them=20written=20mostly=20for=20the=20happy=20path;?= =?UTF-8?q?=20each=20fix=20below=20comes=20with=20the=20test=20that=20fail?= =?UTF-8?q?s=20without=20it.=20Infinite=20scroll:=20a=20later=20page=20tha?= =?UTF-8?q?t=20failed=20turned=20loadingMore=20back=20off,=20which=20made?= =?UTF-8?q?=20a=20new=20IntersectionObserver,=20which=20reports=20a=20sent?= =?UTF-8?q?inel=20still=20in=20view=20at=20once=20=E2=80=94=20so=20an=20of?= =?UTF-8?q?fline=20feed=20asked=20again=20as=20fast=20as=20the=20request?= =?UTF-8?q?=20could=20fail,=20on=20Search,=20Index,=20the=20followed=20fee?= =?UTF-8?q?ds,=20user=20dumps=20and=20user=20playlists=20alike.=20useInfin?= =?UTF-8?q?iteScroll=20now=20waits=201s,=202s,=204s=E2=80=A6=20up=20to=203?= =?UTF-8?q?0s=20while=20the=20sentinel=20stays=20put,=20counted=20from=20t?= =?UTF-8?q?he=20last=20load=20so=20re-renders=20don't=20restart=20it,=20an?= =?UTF-8?q?d=20asks=20at=20once=20again=20when=20the=20sentinel=20leaves?= =?UTF-8?q?=20the=20view=20or=20is=20pushed=20down=20by=20new=20items;=20t?= =?UTF-8?q?he=20fake=20observer=20in=20src/test=20can=20report=20an=20elem?= =?UTF-8?q?ent=20in=20view=20on=20observe(),=20as=20a=20browser's=20does.?= =?UTF-8?q?=20Playlist=20covers:=20the=20row=20was=20updated=20and=20playl?= =?UTF-8?q?ist=5Fupdated=20sent=20before=20the=20file=20was=20written,=20s?= =?UTF-8?q?o=20a=20client=20fetching=20image=3Fv=3D=20in=20between=20?= =?UTF-8?q?cached=20the=20old=20cover=20(or=20a=20404)=20under=20the=20new?= =?UTF-8?q?=20URL=20for=20an=20hour,=20and=20a=20failed=20write=20left=20i?= =?UTF-8?q?mage=5Fmime=20set=20with=20no=20file=20behind=20it.=20The=20fil?= =?UTF-8?q?e=20now=20goes=20in=20first,=20written=20beside=20and=20renamed?= =?UTF-8?q?=20over,=20and=20only=20then=20does=20the=20row=20change.=20Pla?= =?UTF-8?q?ylist=20order:=20the=20server=20wanted=20every=20member=20named?= =?UTF-8?q?,=20but=20GET=20hides=20private=20dumps=20the=20requester=20can?= =?UTF-8?q?'t=20see,=20so=20a=20playlist=20holding=20someone=20else's=20du?= =?UTF-8?q?mp=20that=20went=20private=20=E2=80=94=20or=20one=20a=20moderat?= =?UTF-8?q?or=20reorders=20around=20the=20owner's=20private=20dumps=20?= =?UTF-8?q?=E2=80=94=20could=20never=20be=20reordered.=20The=20requester?= =?UTF-8?q?=20now=20orders=20what=20they=20can=20see=20and=20hidden=20memb?= =?UTF-8?q?ers=20keep=20their=20slots;=20the=20visibility=20rule=20lives?= =?UTF-8?q?=20in=20api/lib/playlist-members.ts,=20shared=20by=20GET,=20reo?= =?UTF-8?q?rder=20and=20the=20socket.=20playlist=5Fdumps=5Fupdated=20also?= =?UTF-8?q?=20sent=20every=20member's=20id,=20private=20ones=20included,?= =?UTF-8?q?=20to=20a=20public=20playlist's=20whole=20audience;=20each=20vi?= =?UTF-8?q?ewer=20now=20gets=20the=20ids=20they=20can=20see,=20plus=20a=20?= =?UTF-8?q?dumpCount=20of=20all=20members=20so=20playlist=20cards=20keep?= =?UTF-8?q?=20the=20server's=20count.=20File=20dumps:=20replacing=20the=20?= =?UTF-8?q?file=20and=20changing=20visibility=20took=20two=20requests,=20s?= =?UTF-8?q?o=20a=20failed=20second=20one=20left=20the=20new=20file=20live?= =?UTF-8?q?=20under=20the=20old=20visibility;=20PUT=20/api/dumps/:id/file?= =?UTF-8?q?=20takes=20isPrivate=20and=20applies=20both=20in=20one=20UPDATE?= =?UTF-8?q?,=20and=20it=20now=20broadcasts=20like=20any=20edit=20(it=20sen?= =?UTF-8?q?t=20nothing,=20so=20other=20viewers=20never=20saw=20a=20new=20t?= =?UTF-8?q?itle).=20Notifications:=20one=20that=20arrived=20while=20the=20?= =?UTF-8?q?list=20was=20loading=20was=20dropped,=20then=20marked=20read=20?= =?UTF-8?q?by=20the=20read-all=20after=20the=20load,=20so=20it=20never=20s?= =?UTF-8?q?howed;=20it's=20kept=20and=20merged=20in,=20once.=20Ranges:=20b?= =?UTF-8?q?ytes=3D0-0,5-6=20got=20a=20416;=20several=20ranges,=20another?= =?UTF-8?q?=20unit=20or=20an=20unparseable=20header=20are=20ignored=20now?= =?UTF-8?q?=20and=20the=20whole=20file=20sent=20with=20a=20200,=20as=20RFC?= =?UTF-8?q?=209110=20allows=20=E2=80=94=20the=20old=20test=20asserted=20th?= =?UTF-8?q?e=20416.?= MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Co-Authored-By: Claude Opus 5.5 --- api/lib/playlist-members.ts | 23 ++++ api/model/interfaces.ts | 3 + api/routes/dumps.ts | 3 + api/routes/files.ts | 16 ++- api/routes/playlists.ts | 34 +++--- api/services/dump-service.ts | 14 ++- api/services/playlist-service.ts | 77 +++++++++---- api/services/ws-service.ts | 24 +++-- api/test/dumps.test.ts | 29 +++++ api/test/playlists.test.ts | 143 +++++++++++++++++++++++++ api/test/uploads.test.ts | 19 +++- api/test/ws.test.ts | 87 +++++++++++++++ src/contexts/WSContext.ts | 1 + src/contexts/WSProvider.tsx | 3 +- src/hooks/useInfiniteScroll.ts | 62 +++++++++-- src/hooks/usePlaylistListSync.ts | 5 +- src/model.ts | 3 + src/pages/DumpEdit.tsx | 12 +-- src/pages/Notifications.tsx | 24 ++++- src/test/contexts/WSProvider.test.tsx | 4 +- src/test/hooks/misc.test.tsx | 81 ++++++++++++++ src/test/hooks/sync.test.tsx | 8 ++ src/test/pages/Dump.test.tsx | 26 ++++- src/test/pages/Notifications.test.tsx | 21 ++++ src/test/pages/PlaylistDetail.test.tsx | 2 +- src/test/pages/Search.test.tsx | 18 ++++ src/test/setup.ts | 15 +++ 27 files changed, 672 insertions(+), 85 deletions(-) create mode 100644 api/lib/playlist-members.ts diff --git a/api/lib/playlist-members.ts b/api/lib/playlist-members.ts new file mode 100644 index 00000000..bfa94418 --- /dev/null +++ b/api/lib/playlist-members.ts @@ -0,0 +1,23 @@ +import type { Playlist } from "../model/interfaces.ts"; + +/** A playlist's dump, with what deciding who may see it takes. */ +export interface PlaylistMember { + id: string; + isPrivate: boolean; + userId: string; +} + +/** + * Whether `viewerId` sees this member when the playlist is shown to them: a + * public dump, or a private one of the owner's own, to the owner. Everyone + * else — moderators too — doesn't see private dumps, and neither does the + * owner see someone else's dump once it has gone private. + */ +export function isMemberVisibleTo( + member: PlaylistMember, + playlist: Pick, + viewerId: string | null | undefined, +): boolean { + return !member.isPrivate || + (member.userId === viewerId && viewerId === playlist.userId); +} diff --git a/api/model/interfaces.ts b/api/model/interfaces.ts index 302bc7c4..a6857c20 100644 --- a/api/model/interfaces.ts +++ b/api/model/interfaces.ts @@ -739,7 +739,10 @@ export interface PlaylistDeletedMessage { export interface PlaylistDumpsUpdatedMessage { 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 UserUpdatedMessage { diff --git a/api/routes/dumps.ts b/api/routes/dumps.ts index cc6514ff..cf835131 100644 --- a/api/routes/dumps.ts +++ b/api/routes/dumps.ts @@ -204,6 +204,9 @@ router.put("/:dumpId/file", authMiddleware, async (ctx) => { formData.has("categoryIds") ? parseCategoryIds(formData.get("categoryIds")) : undefined, + formData.has("isPrivate") + ? formData.get("isPrivate") === "true" + : undefined, ); const responseBody: APIResponse = { success: true, data: updatedDump }; ctx.response.body = responseBody; diff --git a/api/routes/files.ts b/api/routes/files.ts index afb5949b..feac7827 100644 --- a/api/routes/files.ts +++ b/api/routes/files.ts @@ -95,16 +95,14 @@ router.get("/:dumpId", async (ctx) => { if (dump.isPrivate) ctx.response.headers.set("Cache-Control", "private"); ctx.response.headers.set("Accept-Ranges", "bytes"); - const rangeHeader = ctx.request.headers.get("Range"); - - if (rangeHeader) { - const match = rangeHeader.match(/^bytes=(\d*)-(\d*)$/); - if (!match) { - ctx.response.status = 416; - ctx.response.headers.set("Content-Range", `bytes */${total}`); - return; - } + // Only a single byte range is served as one. Anything else — several + // ranges, another unit — is a header RFC 9110 lets us ignore, so the whole + // file goes out with a 200 rather than a 416 for ranges that were fine. + const match = ctx.request.headers.get("Range")?.match( + /^bytes=(\d*)-(\d*)$/, + ); + if (match) { // "bytes=a-b", "bytes=a-" (to the end) or "bytes=-n" (the last n bytes). let start: number; let end: number; diff --git a/api/routes/playlists.ts b/api/routes/playlists.ts index 921a721b..a199506a 100644 --- a/api/routes/playlists.ts +++ b/api/routes/playlists.ts @@ -13,6 +13,7 @@ import { addDumpToPlaylist, createPlaylist, deletePlaylist, + getEditablePlaylist, getPlaylist, getPlaylistImageInfo, getPlaylistMembershipsForDump, @@ -131,23 +132,32 @@ router.post("/:playlistId/image", authMiddleware, async (ctx) => { const data = new Uint8Array(await file.arrayBuffer()); const mime = validateImageUpload(data); - // DB update first (validates ownership and resolves slug → UUID), then file write. - // If file write fails, attempt to clear the mime we just set. - const playlist = setPlaylistImage( + // The file goes in before the row changes. Setting image_mime and bumping + // updated_at announces the cover (the `?v=` on its URL, the socket event), + // so the bytes must already be there: an earlier fetch would cache the old + // cover under the new URL, and a failed write would leave a cover that 404s. + // Written beside and renamed over, so the old cover is never half-replaced. + const { id } = getEditablePlaylist( ctx.params.playlistId, + ctx.state.user.userId, + can(ctx.state.user, "playlist:moderate"), + ); + const filePath = `${PLAYLIST_IMAGES_DIR}/${id}`; + const tmpPath = `${filePath}.${crypto.randomUUID()}.tmp`; + await Deno.mkdir(PLAYLIST_IMAGES_DIR, { recursive: true }); + try { + await Deno.writeFile(tmpPath, data); + await Deno.rename(tmpPath, filePath); + } catch (err) { + await Deno.remove(tmpPath).catch(() => {}); + throw err; + } + const playlist = setPlaylistImage( + id, mime, ctx.state.user.userId, can(ctx.state.user, "playlist:moderate"), ); - const filePath = `${PLAYLIST_IMAGES_DIR}/${playlist.id}`; - await Deno.mkdir(PLAYLIST_IMAGES_DIR, { recursive: true }); - try { - await Deno.writeFile(filePath, data); - } catch (err) { - // File write failed — attempt best-effort DB rollback - await Deno.remove(filePath).catch(() => {}); - throw err; - } ctx.response.body = { success: true, data: playlist }; }); diff --git a/api/services/dump-service.ts b/api/services/dump-service.ts index fc6d5284..410c4b03 100644 --- a/api/services/dump-service.ts +++ b/api/services/dump-service.ts @@ -522,6 +522,9 @@ export async function replaceFileDump( comment: string | undefined, title?: string, categoryIds?: string[], + // Left out, visibility stays as it is. Taken here rather than by a second + // request so a new file and a change of visibility land together. + isPrivate?: boolean, ): Promise { assertFileDumpText(title, comment); if (!isAllowedMime(file.type)) { @@ -556,9 +559,10 @@ export async function replaceFileDump( const newSlug = finalTitle !== dump.title ? makeSlug(finalTitle, dumpId) : dump.slug; + const finalPrivate = isPrivate ?? dump.isPrivate; try { db.prepare( - `UPDATE dumps SET title = ?, slug = ?, file_name = ?, file_mime = ?, file_size = ?, comment = ?, updated_at = ? WHERE id = ?;`, + `UPDATE dumps SET title = ?, slug = ?, file_name = ?, file_mime = ?, file_size = ?, comment = ?, is_private = ?, updated_at = ? WHERE id = ?;`, ).run( finalTitle, newSlug ?? null, @@ -566,6 +570,7 @@ export async function replaceFileDump( file.type, file.size, comment ?? null, + finalPrivate ? 1 : 0, now.toISOString(), dumpId, ); @@ -589,7 +594,7 @@ export async function replaceFileDump( linkAttachments(comment, dumpId); } relinkBacklinks("dump_comment", dumpId, dumpId, comment ?? ""); - return { + const updatedDump: Dump = { ...dump, title: finalTitle, slug: newSlug, @@ -597,8 +602,13 @@ export async function replaceFileDump( fileMime: file.type, fileSize: file.size, comment, + isPrivate: finalPrivate, updatedAt: now, }; + if (updatedDump.isPrivate && !dump.isPrivate) broadcastDumpDeleted(dumpId); + else if (!updatedDump.isPrivate) broadcastDumpUpdated(updatedDump); + if (dump.isPrivate && !updatedDump.isPrivate) broadcastNewDump(updatedDump); + return updatedDump; } export function getDumpsByUser( diff --git a/api/services/playlist-service.ts b/api/services/playlist-service.ts index b9aaf8b9..e894a01b 100644 --- a/api/services/playlist-service.ts +++ b/api/services/playlist-service.ts @@ -28,6 +28,10 @@ import { } from "./notification-service.ts"; import { makeSlug, UUID_RE } from "../lib/slugify.ts"; import { linkAttachments } from "./attachment-service.ts"; +import { + isMemberVisibleTo, + type PlaylistMember, +} from "../lib/playlist-members.ts"; const PLAYLIST_SELECT = `p.*, u.username as owner_username, (SELECT COUNT(*) FROM playlist_dumps pd WHERE pd.playlist_id = p.id) as dump_count @@ -89,9 +93,6 @@ export function getPlaylist( throw new APIException(APIErrorCode.NOT_FOUND, 404, "Playlist not found"); } - const isOwner = requestingUserId === playlist.userId; - - // For public playlists (or when viewed by non-owner), filter out private dumps const rows = db.prepare( `SELECT ${DUMP_COLUMNS_ALIASED}, (SELECT COUNT(*) FROM comments WHERE dump_id = d.id AND deleted = 0) as comment_count @@ -103,8 +104,9 @@ export function getPlaylist( ).all(playlist.id, requestingUserId ?? ""); const dumps: Dump[] = rows.filter(isDumpRow).map(dumpRowToApi); - // Owners always see their own private dumps; strip them for non-owners regardless - const visibleDumps = isOwner ? dumps : dumps.filter((d) => !d.isPrivate); + const visibleDumps = dumps.filter((d) => + isMemberVisibleTo(d, playlist, requestingUserId) + ); return { ...playlist, dumps: visibleDumps }; } @@ -225,9 +227,9 @@ export function deletePlaylist( broadcastPlaylistDeleted(playlist.id, playlist.userId, playlist.isPublic); } -export function setPlaylistImage( +/** The playlist, if the requester may edit it; 404 or 403 otherwise. */ +export function getEditablePlaylist( playlistId: string, - imageMime: string, requestingUserId: string, canModerate: boolean, ): Playlist { @@ -235,6 +237,21 @@ export function setPlaylistImage( if (playlist.userId !== requestingUserId && !canModerate) { throw new APIException(APIErrorCode.UNAUTHORIZED, 403, "Forbidden"); } + return playlist; +} + +// Call once the image file is in place: this is what announces it. +export function setPlaylistImage( + playlistId: string, + imageMime: string, + requestingUserId: string, + canModerate: boolean, +): Playlist { + const playlist = getEditablePlaylist( + playlistId, + requestingUserId, + canModerate, + ); // A new cover is an edit: bumping updated_at also moves the `?v=` the // client puts on the image URL, so a cached old cover isn't shown. db.prepare( @@ -292,8 +309,7 @@ export function addDumpToPlaylist( throw err; } - const dumpIds = getCurrentDumpIds(playlist.id); - broadcastPlaylistDumpsUpdated(playlist, dumpIds); + broadcastPlaylistDumpsUpdated(playlist, getCurrentMembers(playlist.id)); // Followers can't open a private dump, even the owner's own. if (playlist.isPublic && !dump.is_private) { @@ -322,8 +338,7 @@ export function removeDumpFromPlaylist( `DELETE FROM playlist_dumps WHERE playlist_id = ? AND dump_id = ?;`, ).run(playlist.id, dumpId); - const dumpIds = getCurrentDumpIds(playlist.id); - broadcastPlaylistDumpsUpdated(playlist, dumpIds); + broadcastPlaylistDumpsUpdated(playlist, getCurrentMembers(playlist.id)); } export function reorderPlaylist( @@ -338,14 +353,20 @@ export function reorderPlaylist( throw new APIException(APIErrorCode.UNAUTHORIZED, 403, "Forbidden"); } - const currentIds = getCurrentDumpIds(playlist.id); - const currentSet = new Set(currentIds); + // The requester orders what they can see — the playlist as GET gives it to + // them. A member hidden from them (someone else's dump that went private, + // or the owner's private dumps for a moderator) can't be named, so it + // keeps its slot and the visible ones are reordered around it. + const members = getCurrentMembers(playlist.id); + const visibleIds = members + .filter((m) => isMemberVisibleTo(m, playlist, requestingUserId)) + .map((m) => m.id); const newSet = new Set(dumpIds); if ( dumpIds.length !== newSet.size || - currentSet.size !== newSet.size || - !currentIds.every((id) => newSet.has(id)) + visibleIds.length !== newSet.size || + !visibleIds.every((id) => newSet.has(id)) ) { throw new APIException( APIErrorCode.BAD_REQUEST, @@ -354,14 +375,18 @@ export function reorderPlaylist( ); } + let next = 0; + const order = members.map((m) => + isMemberVisibleTo(m, playlist, requestingUserId) ? dumpIds[next++] : m.id + ); const update = db.prepare( `UPDATE playlist_dumps SET position = ? WHERE playlist_id = ? AND dump_id = ?;`, ); - for (let i = 0; i < dumpIds.length; i++) { - update.run(i, playlist.id, dumpIds[i]); + for (let i = 0; i < order.length; i++) { + update.run(i, playlist.id, order[i]); } - broadcastPlaylistDumpsUpdated(playlist, dumpIds); + broadcastPlaylistDumpsUpdated(playlist, getCurrentMembers(playlist.id)); } export function getPlaylistMembershipsForDump( @@ -420,9 +445,17 @@ export function getPlaylistImageInfo( : undefined; } -function getCurrentDumpIds(playlistId: string): string[] { +function getCurrentMembers(playlistId: string): PlaylistMember[] { const rows = db.prepare( - `SELECT dump_id FROM playlist_dumps WHERE playlist_id = ? ORDER BY position ASC;`, - ).all(playlistId) as Array<{ dump_id: string }>; - return rows.map((r) => r.dump_id); + `SELECT pd.dump_id, d.is_private, d.user_id + FROM playlist_dumps pd INNER JOIN dumps d ON d.id = pd.dump_id + WHERE pd.playlist_id = ? ORDER BY pd.position ASC;`, + ).all(playlistId) as Array< + { dump_id: string; is_private: number; user_id: string } + >; + return rows.map((r) => ({ + id: r.dump_id, + isPrivate: Boolean(r.is_private), + userId: r.user_id, + })); } diff --git a/api/services/ws-service.ts b/api/services/ws-service.ts index c45a11e0..1686aa6b 100644 --- a/api/services/ws-service.ts +++ b/api/services/ws-service.ts @@ -8,6 +8,10 @@ import type { ServerToClientMessage, User, } from "../model/interfaces.ts"; +import { + isMemberVisibleTo, + type PlaylistMember, +} from "../lib/playlist-members.ts"; export interface WsClient { socket: WebSocket; @@ -200,15 +204,23 @@ export function broadcastPlaylistDeleted( }); } +// Each viewer gets the members they can see, as GET would list them: the ids +// of private dumps don't go out to the whole audience of a public playlist. export function broadcastPlaylistDumpsUpdated( playlist: Playlist, - dumpIds: string[], + members: PlaylistMember[], ): void { - sendToPlaylistAudience(playlist, { - type: "playlist_dumps_updated", - playlistId: playlist.id, - dumpIds, - }); + for (const client of clients) { + if (!playlist.isPublic && client.userId !== playlist.userId) continue; + send(client.socket, { + type: "playlist_dumps_updated", + playlistId: playlist.id, + dumpIds: members + .filter((m) => isMemberVisibleTo(m, playlist, client.userId)) + .map((m) => m.id), + dumpCount: members.length, + }); + } } export function broadcastUserUpdated( diff --git a/api/test/dumps.test.ts b/api/test/dumps.test.ts index 0c89740f..f88e080d 100644 --- a/api/test/dumps.test.ts +++ b/api/test/dumps.test.ts @@ -603,6 +603,35 @@ test("PUT /api/dumps/:id/file replaces a file dump's contents", async () => { ); }); +test("PUT /api/dumps/:id/file: visibility changes with the file, or stays", async () => { + const owner = await createUser(); + const other = await createUser(); + const { id } = insertDump({ userId: owner.id, fileName: "a.txt" }); + const replace = (fields: Record) => + put(`/api/dumps/${id}/file`, { + auth: owner.auth, + form: formWithFile(PNG, "b.png", "image/png", fields), + }); + const visible = async () => + (await get(`/api/dumps/${id}`, { auth: other.auth })).status === 200; + + let res = await replace({ isPrivate: "true" }); + assertEquals(res.body.data.isPrivate, true); + assertEquals(await visible(), false); + assertEquals((await get(`/api/files/${id}`)).status, 404); + // Left out, it stays private. + res = await replace({}); + assertEquals(res.body.data.isPrivate, true); + assertEquals(await visible(), false); + res = await replace({ isPrivate: "false" }); + assertEquals(res.body.data.isPrivate, false); + assertEquals(await visible(), true); + // A refused replacement changes neither. + res = await replace({ isPrivate: "true", categoryIds: '["nope"]' }); + assertEquals(res.status, 400); + assertEquals(await visible(), true); +}); + test("custom thumbnails: set, served, removed", async () => { const owner = await createUser(); const other = await createUser(); diff --git a/api/test/playlists.test.ts b/api/test/playlists.test.ts index cbb48828..41cd0f26 100644 --- a/api/test/playlists.test.ts +++ b/api/test/playlists.test.ts @@ -11,6 +11,7 @@ import { post, put, test, + TEST_DIR, } from "./helpers.ts"; import { assert, assertEquals, assertMatch } from "@std/assert"; @@ -303,3 +304,145 @@ test("playlist images: upload and serve", async () => { assertEquals(served.status, 200); assertEquals(served.bytes, PNG); }); + +test("playlist images: a failed write leaves the row and the old cover alone", async () => { + const owner = await createUser(); + const { id } = insertPlaylist({ userId: owner.id }); + const dir = `${TEST_DIR}/uploads/playlist-images`; + const row = () => + db.prepare(`SELECT image_mime, updated_at FROM playlists WHERE id = ?;`) + .get(id) as { image_mime: string | null; updated_at: string | null }; + + // No cover yet, and the write fails (a directory sits where the file goes): + // the playlist mustn't claim a cover it hasn't got. + await Deno.mkdir(`${dir}/${id}/blocker`, { recursive: true }); + try { + const failed = await post(`/api/playlists/${id}/image`, { + auth: owner.auth, + form: formWithFile(PNG), + }); + assertEquals(failed.status, 500); + assertEquals(row(), { image_mime: null, updated_at: null }); + assertEquals((await get(`/api/playlists/${id}/image`)).status, 404); + } finally { + await Deno.remove(`${dir}/${id}`, { recursive: true }); + } + + // With a cover in place, a failed replacement keeps serving the old one + // under the old version, and leaves no stray temporary file behind. + await post(`/api/playlists/${id}/image`, { + auth: owner.auth, + form: formWithFile(PNG), + }); + const before = row(); + await Deno.chmod(dir, 0o555); + try { + const failed = await post(`/api/playlists/${id}/image`, { + auth: owner.auth, + form: formWithFile(new Uint8Array([...PNG, 0])), + }); + assertEquals(failed.status, 500); + } finally { + await Deno.chmod(dir, 0o755); + } + assertEquals(row(), before); + assertEquals((await get(`/api/playlists/${id}/image`)).bytes, PNG); + const left = [...Deno.readDirSync(dir)] + .map((e) => e.name) + .filter((name) => name.startsWith(id)); + assertEquals(left, [id]); +}); + +test("playlist images: a replaced cover is served under the new version", async () => { + const owner = await createUser(); + const { id } = insertPlaylist({ userId: owner.id }); + const first = await post(`/api/playlists/${id}/image`, { + auth: owner.auth, + form: formWithFile(PNG), + }); + await new Promise((r) => setTimeout(r, 5)); // a distinct updated_at + const other = new Uint8Array([...PNG, 1, 2, 3]); + const second = await post(`/api/playlists/${id}/image`, { + auth: owner.auth, + form: formWithFile(other), + }); + assert(second.body.data.updatedAt !== first.body.data.updatedAt); + assertEquals((await get(`/api/playlists/${id}/image`)).bytes, other); +}); + +test("reorder: someone else's dump that went private keeps its slot", async () => { + const owner = await createUser(); + const author = await createUser(); + const { id } = insertPlaylist({ userId: owner.id }); + const a = insertDump({ userId: owner.id }); + const theirs = insertDump({ userId: author.id }); + const b = insertDump({ userId: owner.id }); + // Each add goes on top: the order is b, theirs, a. + for (const d of [a, theirs, b]) { + await post(`/api/playlists/${id}/dumps/${d.id}`, { auth: owner.auth }); + } + await put(`/api/dumps/${theirs.id}`, { + auth: author.auth, + json: { isPrivate: true }, + }); + // The owner can't see it, so can't name it: what they see is theirs to order. + assertEquals(ids(await get(`/api/playlists/${id}`, { auth: owner.auth })), [ + b.id, + a.id, + ]); + const res = await put(`/api/playlists/${id}/order`, { + auth: owner.auth, + json: { dumpIds: [a.id, b.id] }, + }); + assertEquals(res.status, 200); + assertEquals(ids(await get(`/api/playlists/${id}`, { auth: owner.auth })), [ + a.id, + b.id, + ]); + // The hidden one hasn't moved, and comes back where it was if made public. + await put(`/api/dumps/${theirs.id}`, { + auth: author.auth, + json: { isPrivate: false }, + }); + assertEquals(ids(await get(`/api/playlists/${id}`)), [a.id, theirs.id, b.id]); + // Naming a hidden id is still not the list the requester sees. + await put(`/api/dumps/${theirs.id}`, { + auth: author.auth, + json: { isPrivate: true }, + }); + assertEquals( + (await put(`/api/playlists/${id}/order`, { + auth: owner.auth, + json: { dumpIds: [a.id, theirs.id, b.id] }, + })).status, + 400, + ); +}); + +test("reorder: a moderator orders around the owner's private dumps", async () => { + const owner = await createUser(); + const mod = await createUser({ role: "moderator" }); + const { id } = insertPlaylist({ userId: owner.id }); + const a = insertDump({ userId: owner.id }); + const priv = insertDump({ userId: owner.id, isPrivate: true }); + const b = insertDump({ userId: owner.id }); + for (const d of [a, priv, b]) { + await post(`/api/playlists/${id}/dumps/${d.id}`, { auth: owner.auth }); + } + assertEquals(ids(await get(`/api/playlists/${id}`, { auth: mod.auth })), [ + b.id, + a.id, + ]); + assertEquals( + (await put(`/api/playlists/${id}/order`, { + auth: mod.auth, + json: { dumpIds: [a.id, b.id] }, + })).status, + 200, + ); + assertEquals(ids(await get(`/api/playlists/${id}`, { auth: owner.auth })), [ + a.id, + priv.id, + b.id, + ]); +}); diff --git a/api/test/uploads.test.ts b/api/test/uploads.test.ts index 85fe75c4..9263c681 100644 --- a/api/test/uploads.test.ts +++ b/api/test/uploads.test.ts @@ -116,14 +116,29 @@ test("GET /api/files/:id: byte ranges", async () => { "bytes=6-2", "bytes=-0", "bytes=-", - "items=0-1", - "bytes=0-1,3-4", ] ) { res = await range(bad); assertEquals(res.status, 416, bad); assertEquals(res.headers.get("content-range"), "bytes */10", bad); } + + // Several ranges, or a unit other than bytes, aren't refused: the header is + // ignored and the whole file sent, as RFC 9110 allows. + for ( + const unsupported of [ + "bytes=0-0,5-6", + "bytes=0-1, 3-4", + "items=0-1", + "bytes=zero-one", + ] + ) { + res = await range(unsupported); + assertEquals(res.status, 200, unsupported); + assertEquals(new TextDecoder().decode(res.bytes), "0123456789"); + assertEquals(res.headers.get("content-range"), null, unsupported); + assertEquals(res.headers.get("content-length"), "10", unsupported); + } }); test("GET /api/files/:id: private files need the owner's media token", async () => { diff --git a/api/test/ws.test.ts b/api/test/ws.test.ts index 88551e45..eb8c2fb1 100644 --- a/api/test/ws.test.ts +++ b/api/test/ws.test.ts @@ -6,6 +6,7 @@ import { createUser, db, formWithFile, + get, insertComment, insertDump, insertPlaylist, @@ -543,3 +544,89 @@ wsTest("a password change closes the user's sockets", async (connect) => { assertExists(res.body.data.token); assertFalse(res.body.data.token === user.token); }); + +wsTest( + "playlist_dumps_updated lists only what each viewer can see", + async (connect) => { + const owner = await createUser(); + const author = await createUser(); + const other = await createUser(); + const pl = insertPlaylist({ userId: owner.id }); + const pub = insertDump({ userId: owner.id }); + const mine = insertDump({ userId: owner.id, isPrivate: true }); + const theirs = insertDump({ userId: author.id, isPrivate: true }); + // Rows straight in: the dumps are private now, as if they went private after + // being added. + for (const [i, d] of [pub, mine, theirs].entries()) { + db.prepare( + `INSERT INTO playlist_dumps (playlist_id, dump_id, position, added_at) + VALUES (?, ?, ?, ?);`, + ).run(pl.id, d.id, i, new Date().toISOString()); + } + const o = await connect(owner); + const a = await connect(author); + const x = await connect(other); + const anon = await connect(); + + const added = insertDump({ userId: owner.id }); + await post(`/api/playlists/${pl.id}/dumps/${added.id}`, { + auth: owner.auth, + }); + // The owner sees their own private dump but not the author's; the author, + // not being the owner, sees neither private one; nor does anyone else. + assertEquals((await o.next("playlist_dumps_updated")).dumpIds, [ + added.id, + pub.id, + mine.id, + ]); + for (const c of [a, x, anon]) { + const msg = await c.next("playlist_dumps_updated"); + assertEquals(msg.dumpIds, [added.id, pub.id]); + // The count is the playlist's, as its dumpCount gives it to everyone. + assertEquals(msg.dumpCount, 4); + } + }, +); + +wsTest( + "a new playlist cover is on disk by the time it's announced", + async (connect) => { + const owner = await createUser(); + const pl = insertPlaylist({ userId: owner.id }); + const c = await connect(); + const cover = new Uint8Array([...PNG, 9, 9, 9]); + const upload = post(`/api/playlists/${pl.id}/image`, { + auth: owner.auth, + form: formWithFile(cover), + }); + // Fetch the image the moment the socket says it changed, as a client would. + const msg = await c.next("playlist_updated"); + const served = await get( + `/api/playlists/${pl.id}/image?v=${msg.playlist.updatedAt}`, + ); + assertEquals(served.status, 200); + assertEquals(served.bytes, cover); + assertEquals((await upload).status, 200); + }, +); + +wsTest( + "replacing a dump's file is broadcast like any edit", + async (connect) => { + const owner = await createUser(); + const other = await createUser(); + const { id } = insertDump({ userId: owner.id, fileName: "a.txt" }); + const x = await connect(other); + const replace = (fields: Record) => + put(`/api/dumps/${id}/file`, { + auth: owner.auth, + form: formWithFile(PNG, "b.png", "image/png", fields), + }); + await replace({ title: "New title" }); + assertEquals((await x.next("dump_updated")).dump.title, "New title"); + await replace({ isPrivate: "true" }); + assertEquals((await x.next("dump_deleted")).dumpId, id); + await replace({ isPrivate: "false" }); + assertEquals((await x.next("dump_created")).dump.id, id); + }, +); diff --git a/src/contexts/WSContext.ts b/src/contexts/WSContext.ts index 400b00ea..f239ed63 100644 --- a/src/contexts/WSContext.ts +++ b/src/contexts/WSContext.ts @@ -27,6 +27,7 @@ export interface PlaylistEvent { playlist?: Playlist; userId?: string; dumpIds?: string[]; + dumpCount?: number; } export interface CommentEvent { diff --git a/src/contexts/WSProvider.tsx b/src/contexts/WSProvider.tsx index 948fca8e..6d08b132 100644 --- a/src/contexts/WSProvider.tsx +++ b/src/contexts/WSProvider.tsx @@ -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; } diff --git a/src/hooks/useInfiniteScroll.ts b/src/hooks/useInfiniteScroll.ts index 6b211d2a..14044582 100644 --- a/src/hooks/useInfiniteScroll.ts +++ b/src/hooks/useInfiniteScroll.ts @@ -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 `
` 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(null); + // Loads asked for since the sentinel last left the view or moved. + const attemptsRef = useRef(0); + const lastLoadAtRef = useRef(0); + const lastTopRef = useRef(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; diff --git a/src/hooks/usePlaylistListSync.ts b/src/hooks/usePlaylistListSync.ts index 9c99dc17..2992191d 100644 --- a/src/hooks/usePlaylistListSync.ts +++ b/src/hooks/usePlaylistListSync.ts @@ -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; } diff --git a/src/model.ts b/src/model.ts index 3ed2d920..a05934d9 100644 --- a/src/model.ts +++ b/src/model.ts @@ -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"; diff --git a/src/pages/DumpEdit.tsx b/src/pages/DumpEdit.tsx index 7937bc23..141b3ae1 100644 --- a/src/pages/DumpEdit.tsx +++ b/src/pages/DumpEdit.tsx @@ -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(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. diff --git a/src/pages/Notifications.tsx b/src/pages/Notifications.tsx index 22f8d184..141022c2 100644 --- a/src/pages/Notifications.tsx +++ b/src/pages/Notifications.tsx @@ -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({ status: "loading" }); + const [state, setState] = useState({ + 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] }; diff --git a/src/test/contexts/WSProvider.test.tsx b/src/test/contexts/WSProvider.test.tsx index e9b48b45..4e53d7cc 100644 --- a/src/test/contexts/WSProvider.test.tsx +++ b/src/test/contexts/WSProvider.test.tsx @@ -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" }); diff --git a/src/test/hooks/misc.test.tsx b/src/test/hooks/misc.test.tsx index 2f201cbb..58784761 100644 --- a/src/test/hooks/misc.test.tsx +++ b/src/test/hooks/misc.test.tsx @@ -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(); + 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(); + intersect(true); + intersect(true); + act(() => vi.advanceTimersByTime(600)); + // A new onLoadMore (the feed's state changed) remakes the observer. + rerender( 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(); + 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(); + intersect(true); + intersect(true); + rerender(); + act(() => vi.advanceTimersByTime(60_000)); + expect(onMore).toHaveBeenCalledTimes(1); + rerender(); + intersect(true); // long overdue: at once + expect(onMore).toHaveBeenCalledTimes(2); + intersect(true); + unmount(); + act(() => vi.advanceTimersByTime(60_000)); + expect(onMore).toHaveBeenCalledTimes(2); + }); }); describe("useScrollLock", () => { diff --git a/src/test/hooks/sync.test.tsx b/src/test/hooks/sync.test.tsx index 5223b2c7..00f3d2d6 100644 --- a/src/test/hooks/sync.test.tsx +++ b/src/test/hooks/sync.test.tsx @@ -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", () => { diff --git a/src/test/pages/Dump.test.tsx b/src/test/pages/Dump.test.tsx index 1722c060..6672fc38 100644 --- a/src/test/pages/Dump.test.tsx +++ b/src/test/pages/Dump.test.tsx @@ -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('.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('.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 () => { diff --git a/src/test/pages/Notifications.test.tsx b/src/test/pages/Notifications.test.tsx index 18b65381..23326fc2 100644 --- a/src/test/pages/Notifications.test.tsx +++ b/src/test/pages/Notifications.test.tsx @@ -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((r) => (answer = r))); + route("POST /api/notifications/read-all", () => ({})); + renderApp(, { 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" diff --git a/src/test/pages/PlaylistDetail.test.tsx b/src/test/pages/PlaylistDetail.test.tsx index 9da0228e..f6facac9 100644 --- a/src/test/pages/PlaylistDetail.test.tsx +++ b/src/test/pages/PlaylistDetail.test.tsx @@ -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(); diff --git a/src/test/pages/Search.test.tsx b/src/test/pages/Search.test.tsx index 9934b021..6a8dc29f 100644 --- a/src/test/pages/Search.test.tsx +++ b/src/test/pages/Search.test.tsx @@ -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")}` })] })); diff --git a/src/test/setup.ts b/src/test/setup.ts index f9f28690..feb4d0b3 100644 --- a/src/test/setup.ts +++ b/src/test/setup.ts @@ -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(); 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(() => {