fix club images

apps/clubs/src/clubs-db.ts

@@ -130,9 +130,9 @@ interface StoredClub extends Club {
 	CreatedAt: string
 	CustomTags?: string[]
 	/**
-	 * The club's gallery images, by slot (the client PUTs to
-	 * `/additionalimage/{index}`). Positional, so a cleared middle slot stays as an
-	 * empty string rather than shifting the images after it.
+	 * The club's gallery image names, in order (the client PUTs to
+	 * `/additionalimage/{index}`). Packed, never sparse: removing one shifts the rest
+	 * up, so the list is always the images the club actually has.
 	 */
 	AdditionalImages?: string[]
 }
@@ -673,39 +673,32 @@ function dedupeTags(tags: string[]): string[] {
 	return out
 }
 
-/**
- * A club's gallery images (stored on the blob). Trailing empty slots are trimmed, so
- * a club with nothing set reads as `[]` while a club with only slot 1 filled still
- * reports `['', 'name.jpg']` — the index a client PUT to is the index it reads back.
- */
+/** A club's gallery image names, in order (stored on the blob; `[]` when it has none). */
 export async function getClubAdditionalImages(db: D1Database, clubId: number): Promise<string[]> {
 	const row = await db
 		.prepare('SELECT data FROM club WHERE club_id = ?1')
 		.bind(clubId)
 		.first<ClubRow>()
-	const images = row === null ? [] : ((JSON.parse(row.data) as StoredClub).AdditionalImages ?? [])
-	let end = images.length
-	while (end > 0 && images[end - 1] === '') end--
-	return images.slice(0, end)
+	return row === null ? [] : ((JSON.parse(row.data) as StoredClub).AdditionalImages ?? [])
 }
 
 /**
- * A club's gallery as the client reads it: the image record behind each filled slot,
- * in slot order. Empty slots are left out (the records carry no index, so a hole
- * would just be a blank image), and a name whose metadata row is missing falls back
- * to a placeholder record so the picture still renders.
+ * A club's gallery as the client reads it: the image record behind each name, in
+ * order. A name whose metadata row is missing falls back to a placeholder record so
+ * the picture still renders.
  */
 export async function getClubGallery(db: D1Database, clubId: number): Promise<SavedImage[]> {
-	const names = (await getClubAdditionalImages(db, clubId)).filter((n) => n !== '')
+	const names = await getClubAdditionalImages(db, clubId)
 	if (names.length === 0) return []
 	const records = await getSavedImagesByNames(db, names)
 	return names.map((name) => records.get(name) ?? placeholderSavedImage(name))
 }
 
 /**
- * Set (or clear, with an empty `imageName`) one of a club's gallery image slots.
- * Returns null when the club doesn't exist. The slot must be in range — callers
- * validate the index before getting here.
+ * Set (or remove, with an empty `imageName`) one of a club's gallery images. The list
+ * stays packed: removing an image shifts the ones after it up, and setting an index
+ * past the end appends rather than leaving a gap. Returns null when the club doesn't
+ * exist; the caller validates the index is in range.
  */
 export async function setClubAdditionalImage(
 	db: D1Database,
@@ -720,11 +713,15 @@ export async function setClubAdditionalImage(
 	if (row === null) return null
 	const stored = JSON.parse(row.data) as StoredClub
 
-	// Pad rather than assign past the end: a sparse array would serialize its holes as
-	// nulls, and the client expects strings in every slot it reads.
 	const images = [...(stored.AdditionalImages ?? [])]
-	while (images.length <= index) images.push('')
-	images[index] = imageName
+	if (imageName === '') {
+		// Removing past the end is a no-op, not an error: the image is already gone.
+		if (index < images.length) images.splice(index, 1)
+	} else if (index < images.length) {
+		images[index] = imageName
+	} else if (images.length < MAX_ADDITIONAL_IMAGES) {
+		images.push(imageName)
+	}
 
 	const updated: StoredClub = { ...stored, AdditionalImages: images }
 	await db

apps/clubs/src/clubs.app.ts

@@ -574,15 +574,15 @@ const app = new Hono<App>()
 		})
 	})
 
-	// One of the club's gallery images, by slot (`/additionalimage/{index}`, 0-based —
-	// the client PUTs the first image to 0, the second to 1). Takes the same
-	// `imageName` the `storage` worker handed back; an empty one clears that slot.
-	// Co-owner or above, like the main image. The slots are positional, so clearing
-	// one doesn't shift the others; they come back on `value.AdditionalImages`.
+	// One of the club's gallery images, by position (`/additionalimage/{index}`, 0-based
+	// — the client PUTs the first image to 0, the second to 1). Takes the same
+	// `imageName` the `storage` worker handed back. Co-owner or above, like the main
+	// image. The list is packed: a PUT past the end appends rather than leaving a gap,
+	// and the images come back in order on `value.AdditionalImages`.
 	//
-	// DELETE removes that slot's image — same thing as PUTting an empty name, with the
-	// intent spelled out. It ignores any body, so it can't accidentally set one, and
-	// deleting an already-empty slot is a no-op rather than an error.
+	// DELETE removes that position's image and shifts the rest up, so there's never a
+	// blank slot in the gallery. It ignores any body, so it can't accidentally set an
+	// image instead, and deleting a position that holds nothing is a no-op.
 	.on(['PUT', 'DELETE'], '/club/:clubId{[0-9]+}/additionalimage/:index{[0-9]+}', async (c) => {
 		const id = await authedId(c)
 		if (id === null) return c.body(null, 401)

apps/clubs/src/test/integration/api.test.ts

@@ -634,29 +634,30 @@ describe('clubs endpoints', () => {
 			expect.objectContaining({ Id: 500, ImageName: first, PlayerId: 7100 }),
 		])
 
-		// Slots are positional: filling 2 while 1 is empty keeps them in that order, and
-		// the empty slot isn't served as a blank image.
+		// The list stays packed: a PUT past the end appends rather than leaving a gap.
 		const third = (await (await setImage(2, 'c.jpg')).json()) as Details
 		expect(names(third.value.AdditionalImages)).toEqual([first, 'c.jpg'])
-		const second = (await (await setImage(1, 'b.jpg')).json()) as Details
-		expect(names(second.value.AdditionalImages)).toEqual([first, 'b.jpg', 'c.jpg'])
+		const second = (await (await setImage(2, 'b.jpg')).json()) as Details
+		expect(names(second.value.AdditionalImages)).toEqual([first, 'c.jpg', 'b.jpg'])
 
-		// Re-PUTting a slot replaces just that image; an empty name clears it. A name with
-		// no image row still renders, as a placeholder record.
+		// Re-PUTting a position replaces just that image. A name with no image row still
+		// renders, as a placeholder record.
 		const replaced = (await (await setImage(0, 'a2.jpg')).json()) as Details
-		expect(names(replaced.value.AdditionalImages)).toEqual(['a2.jpg', 'b.jpg', 'c.jpg'])
+		expect(names(replaced.value.AdditionalImages)).toEqual(['a2.jpg', 'c.jpg', 'b.jpg'])
 		expect(replaced.value.AdditionalImages[0]).toMatchObject({ Id: 0, ImageName: 'a2.jpg' })
+
+		// An empty name removes that image and shifts the rest up — no blank left behind.
 		const cleared = (await (await setImage(1, '')).json()) as Details
-		expect(names(cleared.value.AdditionalImages)).toEqual(['a2.jpg', 'c.jpg'])
+		expect(names(cleared.value.AdditionalImages)).toEqual(['a2.jpg', 'b.jpg'])
 
 		// They're on the club's details payload, for everyone reading the club.
 		const details = (await (
 			await exports.default.fetch(`${ORIGIN}/club/${clubId}/details`)
 		).json()) as { AdditionalImages: Image[] }
-		expect(names(details.AdditionalImages)).toEqual(['a2.jpg', 'c.jpg'])
+		expect(names(details.AdditionalImages)).toEqual(['a2.jpg', 'b.jpg'])
 
-		// DELETE removes a slot's image, ignoring any body, and repeats are no-ops. The
-		// other slots keep their positions, so slot 2 is still slot 2.
+		// DELETE removes that position's image and shifts the rest up, ignoring any body.
+		// Deleting a position that holds nothing is a no-op.
 		const deleteImage = async (index: number, sub = '7100', body?: string) =>
 			exports.default.fetch(`${ORIGIN}/club/${clubId}/additionalimage/${index}`, {
 				method: 'DELETE',
@@ -665,12 +666,14 @@ describe('clubs endpoints', () => {
 			})
 		const dropped = (await (await deleteImage(0, '7100', 'imageName=sneaky.jpg')).json()) as Details
 		expect(dropped).toMatchObject({ error: '', success: true })
-		expect(names(dropped.value.AdditionalImages)).toEqual(['c.jpg'])
+		expect(names(dropped.value.AdditionalImages)).toEqual(['b.jpg'])
+		// Position 1 holds nothing now, so deleting it changes nothing.
 		expect(
-			names(((await (await deleteImage(0)).json()) as Details).value.AdditionalImages)
-		).toEqual(['c.jpg'])
-		const refilled = (await (await setImage(0, 'a3.jpg')).json()) as Details
-		expect(names(refilled.value.AdditionalImages)).toEqual(['a3.jpg', 'c.jpg'])
+			names(((await (await deleteImage(1)).json()) as Details).value.AdditionalImages)
+		).toEqual(['b.jpg'])
+		// A PUT past the end appends, so the club is back to two images.
+		const refilled = (await (await setImage(1, 'a3.jpg')).json()) as Details
+		expect(names(refilled.value.AdditionalImages)).toEqual(['b.jpg', 'a3.jpg'])
 
 		// Same gate as the PUT.
 		expect((await deleteImage(0, '7101')).status).toBe(403)