[lists] fix saved rooms

apps/lists/src/lists.app.ts

@@ -10,6 +10,7 @@ import {
 	getPlayerList,
 	getRecentlyUpdatedRooms,
 	getVisitedRooms,
+	removePlayerListItem,
 } from '@repo/domain'
 import { withCleanSpec, withNotFound, withOnError } from '@repo/hono-helpers'
 import { validateAndGetAccountId } from '@repo/jwt'
@@ -24,15 +25,17 @@ import {
 	AUTHED,
 	ContextualFeaturesAck,
 	CREATOR_ACCOUNT_ID_PARAM,
+	CuratedListEnvelope,
 	CuratedListRead,
-	CuratedListSaved,
 	CuratedListsBulk,
 	form,
 	ITEM_ID_PARAM,
 	json,
+	LIST_ID_PARAM,
 	LIST_IDS_PARAM,
 	LIST_NAME_PARAM,
 	LIST_TYPE_PARAM,
+	REMOVE_ITEM_ID_PARAM,
 	SAVE_LIST_NAME_PARAM,
 	SaveItemBody,
 	UNAUTHORIZED_RESPONSE,
@@ -54,6 +57,24 @@ async function authedId(c: Context<App>): Promise<number | null> {
 	return validateAndGetAccountId(c.req.raw, await c.env.JWT_SECRET.get())
 }
 
+/**
+ * What a list WRITE answers (the save and the remove): the list as it now stands, in the
+ * save's projection — every key the read serves minus `Accessibility`, a real difference
+ * from the read, not an oversight — wrapped in `{ Value, Success, Error, error_id }`. The
+ * reads serve the list bare; don't unify them.
+ *
+ * Spliced as a string because `serializeCuratedList` is what puts the 64-bit `ListId` on the
+ * wire unquoted with every digit intact, and `c.json` would undo that.
+ */
+function listWritten(c: Context<App>, list: CuratedList) {
+	const { Accessibility: _accessibility, ...saved } = list
+	return c.body(
+		`{"Value":${serializeCuratedList(saved)},"Success":true,"Error":null,"error_id":null}`,
+		200,
+		{ 'content-type': 'application/json' }
+	)
+}
+
 /** Results.Unauthorized() equivalent — 401 with empty body. */
 function unauthorized(c: Context<App>) {
 	return c.body(null, 401)
@@ -224,6 +245,18 @@ const PERSONAL_ROW_FEEDS: Record<string, (db: D1Database, accountId: number) =>
 	recentlyvisited: (db, accountId) => getVisitedRooms(db, accountId, 0, LIST_SIZE),
 }
 
+/**
+ * Rows that are a HAND-CURATED list in the `list` table, keyed by row slug and naming the
+ * list behind it. The list is the Coach's (account 1), of the row's requested `type`, so
+ * staff edit a row by editing that list's `list_item`s; its insertion order is the row's.
+ *
+ * The owner is pinned rather than matched on name alone: any player can create a list of any
+ * name through `createlistifneeded`, and a name-only lookup would let one fill the row.
+ */
+const LIST_ROWS: Record<string, string> = {
+	staffpicks: 'staffpicks',
+}
+
 /**
  * What a GENERIC row's ids look like. A `Generic` row is the one kind that can name things
  * of MORE THAN ONE sort in a single list, so the sort travels in the id itself: the client
@@ -731,7 +764,7 @@ const app = new Hono<App>()
 	// that took an owner from the client would let anyone write into anyone's list.
 	//
 	// Answers the list as it now stands rather than an acknowledgement, so the row the client
-	// re-renders is the one this call just changed.
+	// re-renders is the one this call just changed — wrapped, see `listWritten`.
 	.put(
 		'/curatedlists/:name/items/:itemId/createlistifneeded',
 		describeRoute({
@@ -751,15 +784,16 @@ const app = new Hono<App>()
 				're-renders is the one this call just changed. Saving the same item twice leaves it in',
 				'the list once.',
 				'',
-				'The response drops `Accessibility`, which the read keeps. That is a real difference in',
+				'The list comes wrapped in a `{ Value, Success, Error, error_id }` envelope, which the',
-				'what the client is sent, not an oversight — every other key, and their order, is the',
+				'read does not use, and drops `Accessibility`, which the read keeps. Both are real',
-				'read’s.',
+				'differences in what the client is sent, not oversights — every other key, and their',
+				'order, is the read’s.',
 			].join('\n'),
 			security: AUTHED,
 			parameters: [SAVE_LIST_NAME_PARAM, ITEM_ID_PARAM],
 			requestBody: form(SaveItemBody, 'Applied only when the list is created'),
 			responses: {
-				200: json(CuratedListSaved, 'The list as it now stands, without `Accessibility`'),
+				200: json(CuratedListEnvelope, 'The list as it now stands, wrapped'),
 				401: UNAUTHORIZED_RESPONSE,
 			},
 		}),
@@ -785,14 +819,50 @@ const app = new Hono<App>()
 				c.req.param('itemId')
 			)
 
-			// The SAVE's projection of a list drops `Accessibility`; the read's keeps it. That is a
+			return listWritten(c, list)
-			// real difference in what the client is sent, not an oversight — don't unify them.
+		}
-			// Every other key, and their order, is the read's.
+	)
-			const { Accessibility: _accessibility, ...saved } = list
 
-			// Serialized by hand for the same reason the read is: the 64-bit `ListId` has to reach
+	// Remove an item from one of the caller's own lists (`DELETE /curatedlists/:listId/items/:itemId`)
-			// the client unquoted with every digit intact.
+	// — what the client calls to un-save a room. The path names the list by ID rather than by
-			return c.body(serializeCuratedList(saved), 200, { 'content-type': 'application/json' })
+	// name, so unlike the save the owner is not implied by the token: the list must BE the
+	// caller's, or the call is refused. Answers the list as it now stands, exactly as the save
+	// does — see `listWritten`.
+	.delete(
+		'/curatedlists/:listId/items/:itemId',
+		describeRoute({
+			tags: ['Lists', '2025'],
+			summary: 'Remove an item from the caller’s list',
+			description: [
+				'Removes an item from one of the caller’s own lists — what the client calls to un-save',
+				'a room. The path names the list by ID, so the list must belong to the token’s account:',
+				'another player’s list is refused with 403, and an unknown id is 404.',
+				'',
+				'Answers the list as it now stands exactly as the save does: wrapped in a',
+				'`{ Value, Success, Error, error_id }` envelope, without `Accessibility`.',
+				'Removing an item the list doesn’t hold answers the list unchanged, and a list that',
+				'empties is kept rather than deleted.',
+			].join('\n'),
+			security: AUTHED,
+			parameters: [LIST_ID_PARAM, REMOVE_ITEM_ID_PARAM],
+			responses: {
+				200: json(CuratedListEnvelope, 'The list as it now stands, wrapped'),
+				401: UNAUTHORIZED_RESPONSE,
+				403: { description: 'The list is another player’s (empty body)' },
+				404: { description: 'No list has that id (empty body)' },
+			},
+		}),
+		async (c) => {
+			const accountId = await authedId(c)
+			if (accountId === null) return unauthorized(c)
+
+			const listId = Number(c.req.param('listId'))
+			if (!Number.isSafeInteger(listId)) return c.body(null, 404)
+
+			const result = await removePlayerListItem(c.env.DB, accountId, listId, c.req.param('itemId'))
+			if (!result.ok) return c.body(null, result.reason === 'not_owner' ? 403 : 404)
+
+			return listWritten(c, result.list)
 		}
 	)
 
@@ -828,7 +898,9 @@ const app = new Hono<App>()
 				'`recentlyvisited` is per-caller and is the one row that reads the token; without one it',
 				'answers EMPTY rather than 401ing, since canned rooms would claim the caller visited',
 				'rooms they never did — and an empty carousel is what a brand-new account legitimately',
-				'has. A couple of store rows are hand-picked id lists.',
+				'has. A couple of store rows are hand-picked id lists. `staffpicks` serves the Coach’s',
+				'(account 1) list named `staffpicks` of the requested type from the `list` table, in',
+				'insertion order — empty until that list exists.',
 				'',
 				'Every other row — an unknown key included — answers an EMPTY 200 rather than a 404,',
 				'which the client renders as a row that failed to load instead of one it hides.',
@@ -905,6 +977,13 @@ const app = new Hono<App>()
 				return c.json({ Type: echoed, Entities: toEntities(rooms) })
 			}
 
+			// A hand-curated row serves its list's ids as stored. No list yet answers an empty row.
+			const listName = LIST_ROWS[key]
+			if (listName !== undefined) {
+				const list = await getPlayerList(c.env.DB, COACH_ACCOUNT_ID, echoed, listName)
+				return c.json({ Type: echoed, Entities: entities(list?.ItemIds ?? []) })
+			}
+
 			// A row with a live feed behind it serves that; everything else gets the canned
 			// entities. Only the ids travel — the client resolves each room itself — so the
 			// ranking is read for its order and the room blobs are thrown away.

apps/lists/src/openapi.ts

@@ -135,6 +135,27 @@ export const SAVE_LIST_NAME_PARAM: OpenAPIV3_1.ParameterObject = {
 	schema: { type: 'string', example: '__SavedForLater_Rooms' },
 }
 
+/** `{listId}` on the remove — one of the caller's own lists, by id. */
+export const LIST_ID_PARAM: OpenAPIV3_1.ParameterObject = {
+	name: 'listId',
+	in: 'path',
+	required: true,
+	description: 'The list to remove from. It must be the caller’s own.',
+	schema: { type: 'integer', example: 3437 },
+}
+
+/** `{itemId}` on the remove — what comes out of the list, as a string. */
+export const REMOVE_ITEM_ID_PARAM: OpenAPIV3_1.ParameterObject = {
+	name: 'itemId',
+	in: 'path',
+	required: true,
+	description: [
+		'The item to remove, as a string. Removing one the list doesn’t hold answers the list',
+		'unchanged.',
+	].join(' '),
+	schema: { type: 'string', example: '2576' },
+}
+
 /** `{itemId}` on the save — what goes into the list, as a string. */
 export const ITEM_ID_PARAM: OpenAPIV3_1.ParameterObject = {
 	name: 'itemId',
@@ -232,6 +253,18 @@ export const CuratedListRead = z.object({
 /** The SAVE's projection: every key of the read, in the read's order, minus `Accessibility`. */
 export const CuratedListSaved = z.object(CuratedListFields)
 
+/**
+ * What the WRITES answer (`PUT …/createlistifneeded`, `DELETE …/items/{itemId}`): the save's
+ * projection of the list, wrapped in a `{ Value, Success, Error, error_id }` envelope. The
+ * reads serve the list bare.
+ */
+export const CuratedListEnvelope = z.object({
+	Value: CuratedListSaved,
+	Success: z.literal(true),
+	Error: z.null(),
+	error_id: z.null(),
+})
+
 /** `GET /curatedlists/bulk` — a list per id asked for; today one canned list, always. */
 export const CuratedListsBulk = CuratedListRead.array()
 

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

@@ -797,9 +797,12 @@ it('creates the list and adds the item on PUT …/createlistifneeded', async ()
 	// changed — with the id unquoted, as every read of a list serves it.
 	const body = await res.text()
 	expect(body).toMatch(/"ListId":\d+,/)
-	// The SAVE's projection: every key the read serves EXCEPT `Accessibility`, in the read's
+	// Wrapped in the write envelope, around the SAVE's projection: every key the read serves
-	// order. Compared exactly, not loosely, because the missing key is the point.
+	// EXCEPT `Accessibility`, in the read's order. Compared exactly, not loosely, because the
-	const { ListId: _id, ...rest } = JSON.parse(body) as Record<string, unknown>
+	// missing key is the point.
+	const { Value, ...envelope } = JSON.parse(body) as { Value: Record<string, unknown> }
+	expect(envelope).toEqual({ Success: true, Error: null, error_id: null })
+	const { ListId: _id, ...rest } = Value
 	expect(rest).toEqual({
 		CreatorAccountId: 300,
 		Name: '__SavedForLater_Rooms',
@@ -1510,6 +1513,7 @@ it('generates a spec with no dangling $refs', async () => {
 			'/curatedlists',
 			'/curatedlists/bulk',
 			'/curatedlists/{name}/items/{itemId}/createlistifneeded',
+			'/curatedlists/{listId}/items/{itemId}',
 			'/algorithmiclists/{list}',
 			'/contextualfeatures',
 		])
@@ -1518,3 +1522,88 @@ it('generates a spec with no dangling $refs', async () => {
 	expect(Object.keys(spec.paths)).not.toContain('/openapi.json')
 	expect(JSON.stringify(spec).match(/\$ref/g)).toBeNull()
 })
+
+it('serves the Coach’s `staffpicks` list for /algorithmiclists/staffpicks', async () => {
+	// No list yet: an empty row, not a 404.
+	const empty = await SELF.fetch(`${ORIGIN}/algorithmiclists/staffpicks?type=1`)
+	expect(await empty.json()).toEqual({ Type: 1, Entities: [] })
+
+	// A player's list of the same name must not fill the row — only the Coach's does.
+	await SELF.fetch(`${ORIGIN}/curatedlists/staffpicks/items/9/createlistifneeded`, {
+		method: 'PUT',
+		headers: await bearer('42'),
+		body: new URLSearchParams({ type: '1', accessibility: '1' }),
+	})
+	const now = new Date().toISOString()
+	const { list_id } = (await env.DB.prepare(
+		`INSERT INTO list (creator_account_id, list_type, list_name, accessibility, created_at)
+		 VALUES (1, 1, 'StaffPicks', 1, ?1) RETURNING list_id`
+	)
+		.bind(now)
+		.first<{ list_id: number }>())!
+	for (const id of ['7', '3', '5']) {
+		await env.DB.prepare('INSERT INTO list_item (list_id, item_id) VALUES (?1, ?2)')
+			.bind(list_id, id)
+			.run()
+	}
+
+	const res = await SELF.fetch(`${ORIGIN}/algorithmiclists/StaffPicks?type=1`)
+	expect(res.status).toBe(200)
+	expect(await res.json()).toEqual({
+		Type: 1,
+		Entities: ['7', '3', '5'].map((Id) => ({ Id, Context: null })),
+	})
+})
+
+/** The client's un-save call: DELETE the item from the list by id. */
+async function removeFromList(listId: string, itemId: string, headers: Record<string, string>) {
+	return SELF.fetch(`${ORIGIN}/curatedlists/${listId}/items/${itemId}`, {
+		method: 'DELETE',
+		headers,
+	})
+}
+
+it('removes an item from the caller’s list on DELETE …/items/:itemId', async () => {
+	const headers = await bearer('320')
+	await saveForLater('__SavedForLater_Rooms', '953', headers)
+	const saved = await (await saveForLater('__SavedForLater_Rooms', '2576', headers)).text()
+	const listId = /"ListId":(\d+),/.exec(saved)![1]
+
+	const res = await removeFromList(listId, '2576', headers)
+	expect(res.status).toBe(200)
+	const body = await res.text()
+	expect(body).toMatch(new RegExp(`"ListId":${listId},`))
+	const { Value, ...envelope } = JSON.parse(body) as { Value: Record<string, unknown> }
+	expect(envelope).toEqual({ Success: true, Error: null, error_id: null })
+	const { ListId: _id, ...rest } = Value
+	expect(rest).toEqual({
+		CreatorAccountId: 320,
+		Name: '__SavedForLater_Rooms',
+		Description: null,
+		ImageName: 'DefaultRoomImage.jpg',
+		Type: 1,
+		ItemIds: ['953'],
+		CreatedAt: expect.any(String),
+	})
+
+	// Removing the last item keeps the list; removing an absent one answers it unchanged.
+	await removeFromList(listId, '953', headers)
+	const again = await removeFromList(listId, '953', headers)
+	expect(again.status).toBe(200)
+	expect((await again.json()) as { Value: { ItemIds: string[] } }).toMatchObject({
+		Value: { ItemIds: [] },
+	})
+	expect(await readList(320)).toMatchObject({ ItemIds: [] })
+})
+
+it('refuses to remove from another player’s list, or without a token', async () => {
+	const saved = await (
+		await saveForLater('__SavedForLater_Rooms', '953', await bearer('321'))
+	).text()
+	const listId = /"ListId":(\d+),/.exec(saved)![1]
+
+	expect((await removeFromList(listId, '953', await bearer('322'))).status).toBe(403)
+	expect((await removeFromList(listId, '953', {})).status).toBe(401)
+	expect((await removeFromList('999999', '953', await bearer('321'))).status).toBe(404)
+	expect((await readList(321)).ItemIds).toEqual(['953'])
+})

packages/domain/src/lists-db.ts

@@ -223,3 +223,41 @@ export async function getPlayerList(
 	if (row === null) return undefined
 	return toCuratedList(row, await getListItems(db, row.list_id))
 }
+
+/** What {@link removePlayerListItem} found: the list as it now stands, or why it can't be. */
+export type RemoveListItemResult =
+	{ ok: true; list: CuratedList } | { ok: false; reason: 'not_found' | 'not_owner' }
+
+/**
+ * Remove an item from a list the caller owns — `DELETE /curatedlists/:listId/items/:itemId`,
+ * which the client calls to un-save a room. Unlike the save, the path names the list by ID,
+ * so ownership has to be CHECKED rather than implied: the list must be the caller's.
+ *
+ * Removing an item the list doesn't hold is not an error — the list already says what the
+ * caller wants it to — and answers the list unchanged. The list itself is kept when it
+ * empties: the client created it and expects to find it again by name.
+ */
+export async function removePlayerListItem(
+	db: D1Database,
+	accountId: number,
+	listId: number,
+	itemId: string
+): Promise<RemoveListItemResult> {
+	const row = await db
+		.prepare(
+			`SELECT list_id, creator_account_id, list_type, list_name, list_description,
+			        image_name, accessibility, created_at
+			 FROM list WHERE list_id = ?1`
+		)
+		.bind(listId)
+		.first<ListRow>()
+	if (row === null) return { ok: false, reason: 'not_found' }
+	if (row.creator_account_id !== accountId) return { ok: false, reason: 'not_owner' }
+
+	await db
+		.prepare('DELETE FROM list_item WHERE list_id = ?1 AND item_id = ?2')
+		.bind(listId, itemId)
+		.run()
+
+	return { ok: true, list: toCuratedList(row, await getListItems(db, listId)) }
+}