diff --git a/apps/lists/migrations/0001_curated_list.sql b/apps/lists/migrations/0001_curated_list.sql new file mode 100644 index 0000000..8d9c362 --- /dev/null +++ b/apps/lists/migrations/0001_curated_list.sql @@ -0,0 +1,50 @@ +-- Player-owned curated lists — the playlists a player builds themselves, of which +-- "Saved for Later" (`__SavedForLater_Rooms`) is the one the client creates on its own and +-- reads back through `GET /curatedlists?creatorAccountId=&type=&name=`. Generated from +-- packages/domain/src/lists-db.ts (CURATED_LIST_SCHEMA_DDL) — keep in sync. +-- +-- Until now that endpoint only ever served the static captures in +-- static/curated-lists.json, and a name it had nothing under fell back to the page's +-- default list — so asking for a player's Saved for Later answered the Play/Explore rows. +-- +-- A list is GENERIC: `list_type` is the ListEntityType (1 = Rooms), it says what the +-- `item_id`s ARE, and nothing here interprets them — the client resolves each id against +-- the service that type names. Hence `item_id` TEXT: a list of rooms carries room ids, a +-- list of discovery sections carries section keys, and one column holds both. + +-- `list_id` is an ordinary autoincrement integer. The reference's own ids run to 18 digits +-- (624765592684307326) and the static captures still carry theirs verbatim, but nothing +-- requires a list this server MINTS to look like that — and a small id stays well inside +-- what a JS number holds exactly, so it cannot be rounded on its way through D1 or JSON. +-- AUTOINCREMENT rather than a bare rowid alias: a list id is handed to the client, so a +-- deleted list's id must not later be handed out again for a different list. +CREATE TABLE IF NOT EXISTS list ( + list_id INTEGER PRIMARY KEY AUTOINCREMENT, + creator_account_id INTEGER NOT NULL, + list_type INTEGER NOT NULL, + list_name TEXT NOT NULL, + list_name_lower TEXT GENERATED ALWAYS AS (lower(list_name)) VIRTUAL, + list_description TEXT, + image_name TEXT NOT NULL DEFAULT '', + accessibility INTEGER NOT NULL DEFAULT 1, + created_at TEXT NOT NULL + ); + +-- The lookup the client actually makes, all three keys at once. UNIQUE because that triple +-- is a list's identity: the client asks for `__SavedForLater_Rooms` by name expecting the +-- one it has been appending to, so a player must never end up with two. Folded, since the +-- casing that reaches us is the client's. +CREATE UNIQUE INDEX IF NOT EXISTS idx_list_owner_type_name + ON list (creator_account_id, list_type, list_name_lower); +CREATE INDEX IF NOT EXISTS idx_list_creator ON list (creator_account_id); + +-- A list's contents. Insertion order is preserved by the surrogate key and is the order the +-- ItemIds array is served in. UNIQUE on the pair: saving the same room twice is a no-op, +-- not a carousel showing it twice. +CREATE TABLE IF NOT EXISTS list_item ( + list_item_id INTEGER PRIMARY KEY AUTOINCREMENT, + list_id INTEGER NOT NULL, + item_id TEXT NOT NULL + ); +CREATE UNIQUE INDEX IF NOT EXISTS idx_list_item_pair ON list_item (list_id, item_id); +CREATE INDEX IF NOT EXISTS idx_list_item_list ON list_item (list_id); diff --git a/apps/lists/package.json b/apps/lists/package.json index 7efe4fa..4ffb202 100644 --- a/apps/lists/package.json +++ b/apps/lists/package.json @@ -12,6 +12,7 @@ "deploy": "run-wrangler-deploy", "dev": "run-wrangler-dev", "fix:workers-types": "run-wrangler-types", + "migrate": "run-wrangler-migrate", "test": "run-vitest" }, "dependencies": { diff --git a/apps/lists/src/curated-lists.ts b/apps/lists/src/curated-lists.ts index d7acd96..661af89 100644 --- a/apps/lists/src/curated-lists.ts +++ b/apps/lists/src/curated-lists.ts @@ -1,10 +1,15 @@ +import { DEFAULT_LIST_IMAGE } from '@repo/domain' + import curatedLists from '../static/curated-lists.json' +import type { CuratedList } from '@repo/domain' + +export type { CuratedList } + /** - * Which PAGE a curated list belongs to — the `Type` on the list and the `?type=` the client - * asks with. Not to be confused with the entity-type enum the algorithmic lists echo: this - * one names a page source (the Watch home, the store's Featured tab, …), and the list it - * keys is that page's row set. + * The PAGE-SOURCE enum, kept for reference. NOT what a list's `Type` is — see the note on + * `resolveCuratedList`: the captures and the client's own queries both put the + * `ListEntityType` there (what the `ItemIds` ARE), not the page. Nothing reads this. * * `None` is the client's unset sentinel; it never reaches the wire, and nothing is captured * under it. @@ -30,26 +35,6 @@ export const CuratedListType = { None: 999, } as const -/** - * One curated list as the client parses it. `Description` may be null but `ImageName` must - * be a STRING — the client reads it straight into a string field — and `ItemIds` are - * strings even where they stand for numeric ids. - * - * `ListId` is a string HERE ONLY, and never reaches the client as one: see - * `serializeCuratedList`. `Accessibility` rides along from the captures untouched. - */ -export interface CuratedList { - ListId: string - CreatorAccountId: number - Name: string - Description: string | null - ImageName: string - Type: number - ItemIds: string[] - Accessibility?: number - CreatedAt: string -} - /** * Every list this server serves, all of them in `static/curated-lists.json` — drop a * capture into that array and it is served; nothing else needs editing. (One file per list @@ -121,9 +106,56 @@ const DEFAULT_CURATED_LIST = BY_NAME.get(nameKey('Discovery.PageSource.PlayExplore')) ?? CURATED_LISTS[0] /** - * The list behind `GET /curatedlists?creatorAccountId=&type=&name=`, resolved most-specific - * first and never empty. `type` is the page asking (see `CuratedListType`) and dictates the - * name, so the two normally agree; when they don't, the page's default list still answers. + * The prefix the reference gives a list the CLIENT owns and creates for itself, rather than + * one a person named — `__SavedForLater_Rooms` is the one in play. It matters because these + * are the only names that must be allowed to come back EMPTY: see `resolveCuratedList`. + */ +const RESERVED_LIST_PREFIX = '__' + +/** Whether a name is one of the client's own reserved playlists rather than a curated page. */ +export function isReservedListName(name: string | undefined): boolean { + return (name ?? '').startsWith(RESERVED_LIST_PREFIX) +} + +/** + * The list a reserved name answers when nobody owns one yet — the name asked for, with no + * items. A player who has saved nothing has an empty Saved for Later, not somebody else's + * list; `CreatorAccountId` is echoed from the query so the client still sees the list it + * asked for. `ListId` is 0: nothing was stored, so there is no id to hand back, and the + * client's field is a non-nullable number. + */ +function emptyReservedList(creatorAccountId: string | undefined, type: number, name: string) { + return { + ListId: '0', + CreatorAccountId: Number.parseInt(creatorAccountId ?? '', 10) || 0, + Name: name, + Description: null, + ImageName: DEFAULT_LIST_IMAGE, + Type: type, + ItemIds: [], + Accessibility: 1, + CreatedAt: new Date(0).toISOString(), + } satisfies CuratedList +} + +/** + * The list behind `GET /curatedlists?creatorAccountId=&type=&name=` once D1 has been asked + * and had nothing — the static captures, resolved most-specific first and never empty. + * + * `type` is the `ListEntityType`: what the `ItemIds` ARE. (The client asks for + * `__SavedForLater_Rooms` with `type=1`, Rooms, and every capture here is `type=7`, + * DiscoverySection — which is exactly what their ItemIds hold. It is NOT the page-source + * enum, whose 7 is PlayCategories and would make two of the three captures wrong.) + * + * The fallback chain ends at a real list because an empty body or a 404 renders as a blank + * page, and the store's Featured tab depends on it: it asks for its rows by the reference's + * numeric list id (`name=17859340`), which is nothing's name here. + * + * A RESERVED name is the exception, and stops before that fallback. `__SavedForLater_Rooms` + * is a name this server is meant to have nothing under until the player saves something, so + * falling through would answer their empty Saved for Later row with the Play/Explore rows — + * the default list, under someone else's heading. An empty list hides the row instead, which + * is what its `minItemsToShowSection` asks for. */ export function resolveCuratedList( creatorAccountId: string | undefined, @@ -138,6 +170,9 @@ export function resolveCuratedList( (hasType ? BY_CREATOR_TYPE_NAME.get(`${creatorAccountId}/${parsedType}/${key}`) : undefined) ?? (hasType ? BY_TYPE_NAME.get(`${parsedType}/${key}`) : undefined) ?? BY_NAME.get(key) ?? + (isReservedListName(name) + ? emptyReservedList(creatorAccountId, hasType ? parsedType : 0, name ?? '') + : undefined) ?? (hasType ? BY_TYPE.get(parsedType) : undefined) ?? DEFAULT_CURATED_LIST ) diff --git a/apps/lists/src/lists.app.ts b/apps/lists/src/lists.app.ts index 6387d5c..74bc153 100644 --- a/apps/lists/src/lists.app.ts +++ b/apps/lists/src/lists.app.ts @@ -1,14 +1,22 @@ import { Hono } from 'hono' import { useWorkersLogger } from 'workers-tagged-logger' -import { getHotRooms, getNewRooms, getRecentlyUpdatedRooms, getVisitedRooms } from '@repo/domain' +import { + Accessibility, + addPlayerListItem, + getHotRooms, + getNewRooms, + getPlayerList, + getRecentlyUpdatedRooms, + getVisitedRooms, +} from '@repo/domain' import { withNotFound, withOnError } from '@repo/hono-helpers' import { validateAndGetAccountId } from '@repo/jwt' import { resolveCuratedList, serializeCuratedList } from './curated-lists' import type { Context } from 'hono' -import type { Room } from '@repo/domain' +import type { CuratedList, Room } from '@repo/domain' import type { App } from './context' /** @@ -61,13 +69,7 @@ const MAX_LIST_ENTITY_TYPE = 255 * so it is null on every one rather than a made-up context the client would carry into * telemetry. */ -const ALGORITHMIC_LIST_ENTITIES: Array<{ Id: string; Context: string | null }> = [ - '2', - '3', - '4', - '5', - '6', -].map((Id) => ({ Id, Context: null })) +const ALGORITHMIC_LIST_ENTITIES: Array<{ Id: string; Context: string | null }> = [].map((Id) => ({ Id, Context: null })) /** How many rooms a row carries. A discovery carousel shows a page, not the world. */ const LIST_SIZE = 20 @@ -188,6 +190,56 @@ const PERSONAL_ROW_FEEDS: Record */ const DEFAULT_ALGORITHMIC_LIST_TYPE = ListEntityType.Rooms +/** + * The stored list the query names, if a player owns one. Undefined when any of the three + * keys is missing or unparseable — a player list is owned by an account and typed, so a + * query that names neither cannot be asking for one, and there is no read to make. + * + * Not auth-gated: the client asks for its own lists by passing its account id rather than by + * being logged in, `Accessibility` is a property of the list rather than of the reader, and + * the endpoint has never taken a token. A list read here is only ever ids the client then + * resolves itself. + */ +async function ownedList( + c: Context, + creatorAccountId: string | undefined, + type: string | undefined, + name: string | undefined +): Promise { + const accountId = Number.parseInt(creatorAccountId ?? '', 10) + const listType = Number.parseInt(type ?? '', 10) + if (!Number.isInteger(accountId) || !Number.isInteger(listType) || !name) return undefined + + return getPlayerList(c.env.DB, accountId, listType, name) +} + +/** + * One field of a form-urlencoded body, matched case-insensitively and falling back to the + * query string. The client puts this call's parameters in the BODY (`accessibility=0&type=1`), + * but the same parameters ride the query string everywhere else on this worker, and a PUT + * whose body failed to parse would otherwise silently create a list with the wrong type. + */ +function bodyField( + body: Record, + c: Context, + name: string +): string | undefined { + const key = Object.keys(body).find((k) => k.toLowerCase() === name.toLowerCase()) + const value = key === undefined ? undefined : body[key] + return typeof value === 'string' ? value : c.req.query(name) +} + +/** An integer field, or `fallback` when it is absent or not one. */ +function intField( + body: Record, + c: Context, + name: string, + fallback: number +): number { + const parsed = Number.parseInt(bodyField(body, c, name) ?? '', 10) + return Number.isInteger(parsed) ? parsed : fallback +} + const app = new Hono() .use( '*', @@ -226,35 +278,79 @@ const app = new Hono() ]) }) - // The curated list behind a discovery page (`GET /curatedlists`). The client asks with - // `?creatorAccountId=&type=&name=` and reads back ONE list object — not a collection. - // `type` is the page asking (`CuratedListType`: the Watch home, the store's Featured tab, - // …) and dictates the name it asks under, so the two normally agree. + // One curated list (`GET /curatedlists?creatorAccountId=&type=&name=`). The client reads + // back ONE list object — not a collection — and asks for two different things through the + // same three parameters: // - // Nothing curates lists here, so every list is a static capture in - // `static/curated-lists.json`. `resolveCuratedList` matches most-specific first and - // always answers something: a name this server has nothing under falls back to the page's - // default list rather than 404ing or answering empty, either of which renders as a blank - // page. That fallback is load-bearing — the store's Featured page asks for its rows by the - // reference's numeric list id (`name=17859340`), which is nothing's name here. + // - A discovery PAGE's row set, which is a static capture in `static/curated-lists.json` + // (`ItemIds` are the discovery section keys the page is built from, not room ids). + // - A PLAYER's own playlist, which lives in D1 — the `list` / `list_item` tables this + // worker owns. `__SavedForLater_Rooms` is the one the client creates for itself: the + // Play menu's "Saved for Later" row is `MyPlaylistByName` pointed at that name, and it + // is asked for with the player's own id and `type=1` (Rooms), so its `ItemIds` are + // room ids. // - // `ItemIds` are the discovery ROWS the page is built from (the section keys the `discovery` - // worker serves), not room or item ids — the client resolves each one itself. + // D1 is asked FIRST, so a player's own list wins over a capture that happens to share its + // name — the captures are this server's fixtures and a player's list is their data. + // Nothing else distinguishes the two requests: both are the same three parameters, and a + // name nobody owns still has to answer something (see `resolveCuratedList`). .get('/curatedlists', async (c) => { + const creatorAccountId = c.req.query('creatorAccountId') + const type = c.req.query('type') + const name = c.req.query('name') + + const list = + (await ownedList(c, creatorAccountId, type, name)) ?? + resolveCuratedList(creatorAccountId, type, name) + // Serialized by hand rather than through `c.json`: the reference's `ListId`s are // 64-bit and are carried as strings so their digits survive being parsed — see // `serializeCuratedList`, which puts them back on the wire as numbers. - return c.body( - serializeCuratedList( - resolveCuratedList( - c.req.query('creatorAccountId'), - c.req.query('type'), - c.req.query('name') - ) - ), - 200, - { 'content-type': 'application/json' } + return c.body(serializeCuratedList(list), 200, { 'content-type': 'application/json' }) + }) + + // Save an item into one of the caller's own lists, creating the list if they don't have + // it yet (`PUT /curatedlists/:name/items/:itemId/createlistifneeded`) — what the client + // calls when someone saves a room for later. The path names the list and the item + // (`/curatedlists/__SavedForLater_Rooms/items/953/createlistifneeded`), and the form body + // carries `accessibility` and `type`. + // + // AUTH-GATED, and the owner is the TOKEN's account: unlike the read, this call names no + // `creatorAccountId`, so the only account it could mean is the caller's — and a route + // 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. + .put('/curatedlists/:name/items/:itemId/createlistifneeded', async (c) => { + const accountId = await authedId(c) + if (accountId === null) return unauthorized(c) + + const body = await c.req.parseBody().catch(() => ({}) as Record) + const list = await addPlayerListItem( + c.env.DB, + { + creatorAccountId: accountId, + name: c.req.param('name'), + // The `ListEntityType`, saying what the item ids in this list ARE. Rooms when the + // body names none: every list the client creates this way is a room list, and the + // type is part of the list's identity, so guessing another would strand the list + // where the client's own read (`?type=1`) can't find it. + type: intField(body, c, 'type', ListEntityType.Rooms), + // PRIVATE by default. A list a player builds for themselves is theirs to see; + // the client sends `accessibility=0` and this only applies on creation anyway. + accessibility: intField(body, c, 'accessibility', Accessibility.Private), + }, + c.req.param('itemId') ) + + // The SAVE's projection of a list drops `Accessibility`; the read's keeps it. That is a + // 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 + // the client unquoted with every digit intact. + return c.body(serializeCuratedList(saved), 200, { 'content-type': 'application/json' }) }) // One discovery ROW's contents (`GET /algorithmiclists/:list?type=1`). `:list` is the row diff --git a/apps/lists/src/test/integration/api.test.ts b/apps/lists/src/test/integration/api.test.ts index 6467e71..c3abb5a 100644 --- a/apps/lists/src/test/integration/api.test.ts +++ b/apps/lists/src/test/integration/api.test.ts @@ -2,6 +2,7 @@ import { adminSecretsStore, env, SELF } from 'cloudflare:test' import { beforeAll, expect, it } from 'vitest' import { + CURATED_LIST_SCHEMA_DDL, PRESENCE_SCHEMA_DDL, ROOM_SCHEMA_DDL, seedRoomWithSubRooms, @@ -25,6 +26,8 @@ beforeAll(async () => { for (const stmt of ROOM_SCHEMA_DDL) await env.DB.prepare(stmt).run() for (const stmt of SUBROOM_SCHEMA_DDL) await env.DB.prepare(stmt).run() for (const stmt of PRESENCE_SCHEMA_DDL) await env.DB.prepare(stmt).run() + // The player-owned curated lists, which this worker owns. + for (const stmt of CURATED_LIST_SCHEMA_DDL) await env.DB.prepare(stmt).run() // Creation order and publish order are deliberately near-REVERSES of each other, so the // `new` row and the `recentlyupdated` row can't both be passing on the same ordering. @@ -312,6 +315,298 @@ it('falls back to the default list for a name it has nothing under', async () => } }) +/** + * Store a player's own list and its items, the way the (not yet written) save-for-later + * mutation would. Returns the id it was stored under. + */ +async function seedPlayerList( + list: { + creatorAccountId: number + type: number + name: string + description?: string | null + imageName?: string + accessibility?: number + createdAt?: string + }, + itemIds: string[] +): Promise { + const row = await env.DB.prepare( + `INSERT INTO list (creator_account_id, list_type, list_name, list_description, + image_name, accessibility, created_at) + VALUES (?1, ?2, ?3, ?4, ?5, ?6, ?7) + RETURNING list_id` + ) + .bind( + list.creatorAccountId, + list.type, + list.name, + list.description ?? null, + list.imageName ?? 'DefaultRoomImage.jpg', + list.accessibility ?? 1, + list.createdAt ?? '2025-04-23T18:27:03.2643786Z' + ) + .first<{ list_id: number }>() + const listId = row!.list_id + for (const itemId of itemIds) { + await env.DB.prepare('INSERT INTO list_item (list_id, item_id) VALUES (?1, ?2)') + .bind(listId, itemId) + .run() + } + return listId +} + +it('serves a player’s own __SavedForLater_Rooms list out of D1', async () => { + const listId = await seedPlayerList( + { + creatorAccountId: 205, + type: 1, + name: '__SavedForLater_Rooms', + description: 'Something', + }, + ['3', '4', '8'] + ) + + const res = await SELF.fetch( + `${ORIGIN}/curatedlists?creatorAccountId=205&type=1&name=__SavedForLater_Rooms` + ) + expect(res.status).toBe(200) + const body = await res.text() + // A stored id reaches the client the same way a captured one does: unquoted. + expect(body).toContain(`"ListId":${listId},`) + + const { ListId: _id, ...rest } = JSON.parse(body) as Record + expect(rest).toEqual({ + CreatorAccountId: 205, + Name: '__SavedForLater_Rooms', + Description: 'Something', + ImageName: 'DefaultRoomImage.jpg', + // The ListEntityType: 1 = Rooms, so the ItemIds are room ids the client resolves + // against the rooms worker. ItemIds are STRINGS even here, as in every capture. + Type: 1, + ItemIds: ['3', '4', '8'], + Accessibility: 1, + CreatedAt: '2025-04-23T18:27:03.2643786Z', + }) +}) + +it('serves a player list in the order its items were added', async () => { + await seedPlayerList({ creatorAccountId: 206, type: 1, name: '__SavedForLater_Rooms' }, [ + '8', + '2', + '7', + ]) + const res = await SELF.fetch( + `${ORIGIN}/curatedlists?creatorAccountId=206&type=1&name=__SavedForLater_Rooms` + ) + // Insertion order, not sorted and not the row order D1 happens to return: the ItemIds + // array IS the display order of the carousel. + expect(((await res.json()) as { ItemIds: string[] }).ItemIds).toEqual(['8', '2', '7']) +}) + +it('keeps one player’s list out of another’s, and one list type out of another', async () => { + await seedPlayerList({ creatorAccountId: 207, type: 1, name: '__SavedForLater_Rooms' }, ['3']) + + // The same name for a different player is a different list — and player 208 has none, so + // theirs comes back EMPTY rather than 207's. + const other = await SELF.fetch( + `${ORIGIN}/curatedlists?creatorAccountId=208&type=1&name=__SavedForLater_Rooms` + ) + expect((await other.json()) as unknown).toMatchObject({ + CreatorAccountId: 208, + Name: '__SavedForLater_Rooms', + ItemIds: [], + }) + + // Same owner and name, different entity type: also a different list. The type says what + // the ids ARE, so serving room ids to a request for items would resolve them against the + // wrong service. + const wrongType = await SELF.fetch( + `${ORIGIN}/curatedlists?creatorAccountId=207&type=4&name=__SavedForLater_Rooms` + ) + expect(((await wrongType.json()) as { ItemIds: string[] }).ItemIds).toEqual([]) +}) + +it('matches a player list’s name case-insensitively', async () => { + await seedPlayerList({ creatorAccountId: 209, type: 1, name: '__SavedForLater_Rooms' }, ['4']) + const res = await SELF.fetch( + `${ORIGIN}/curatedlists?creatorAccountId=209&type=1&name=__savedforlater_rooms` + ) + expect(((await res.json()) as { ItemIds: string[] }).ItemIds).toEqual(['4']) +}) + +it('answers an unowned reserved list EMPTY rather than with the default page', async () => { + // The bug this replaces: nothing is captured under `__SavedForLater_Rooms` and nothing + // is captured under type 1 either, so the fallback chain used to end at the default list + // — and a player who had saved nothing got the Play/Explore rows under a "Saved for + // Later" heading. A reserved name stops before that fallback. + const res = await SELF.fetch( + `${ORIGIN}/curatedlists?creatorAccountId=999&type=1&name=__SavedForLater_Rooms` + ) + expect(res.status).toBe(200) + const list = (await res.json()) as { Name: string; ItemIds: string[]; ImageName: string } + expect(list.Name).toBe('__SavedForLater_Rooms') + expect(list.ItemIds).toEqual([]) + // Still a well-formed list: the client parses ImageName into a non-nullable string. + expect(typeof list.ImageName).toBe('string') +}) + +it('still falls back to the default page for a NON-reserved unknown name', async () => { + // The reserved-name carve-out must not swallow the store's Featured lookup, which asks + // by numeric list id and depends on the default answering. + const explore = await ( + await SELF.fetch(`${ORIGIN}/curatedlists?name=Discovery.PageSource.PlayExplore`) + ).text() + const featured = await SELF.fetch( + `${ORIGIN}/curatedlists?creatorAccountId=1&type=4&name=17859340` + ) + expect(await featured.text()).toBe(explore) +}) + +it('prefers a player’s stored list over a capture of the same name', async () => { + // The captures are this server's fixtures; a stored list is a player's own data, so D1 + // is asked first. + await seedPlayerList( + { creatorAccountId: 210, type: 7, name: 'Discovery.PageSource.PlayExplore' }, + ['Rooms_MostPopular'] + ) + const res = await SELF.fetch( + `${ORIGIN}/curatedlists?creatorAccountId=210&type=7&name=Discovery.PageSource.PlayExplore` + ) + expect(((await res.json()) as { ItemIds: string[] }).ItemIds).toEqual(['Rooms_MostPopular']) + + // …and the capture still answers for the account that owns it. + const captured = await SELF.fetch( + `${ORIGIN}/curatedlists?creatorAccountId=1&type=7&name=Discovery.PageSource.PlayExplore` + ) + expect(((await captured.json()) as { ItemIds: string[] }).ItemIds.length).toBe(10) +}) + +/** The client's save-for-later call: PUT the item into the named list, body-encoded. */ +async function saveForLater( + name: string, + itemId: string, + headers: Record, + body = 'accessibility=0&type=1' +) { + return SELF.fetch(`${ORIGIN}/curatedlists/${name}/items/${itemId}/createlistifneeded`, { + method: 'PUT', + headers: { ...headers, 'Content-Type': 'application/x-www-form-urlencoded' }, + body, + }) +} + +/** Read a player's list back the way the client does. */ +async function readList(accountId: number, type = 1, name = '__SavedForLater_Rooms') { + const res = await SELF.fetch( + `${ORIGIN}/curatedlists?creatorAccountId=${accountId}&type=${type}&name=${name}` + ) + expect(res.status).toBe(200) + return (await res.json()) as { + CreatorAccountId: number + Name: string + Type: number + ItemIds: string[] + Accessibility: number + ImageName: string + } +} + +it('creates the list and adds the item on PUT …/createlistifneeded', async () => { + const res = await saveForLater('__SavedForLater_Rooms', '953', await bearer('300')) + expect(res.status).toBe(200) + + // Answers the list as it now stands — the row the client re-renders is the one this call + // 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 + // order. Compared exactly, not loosely, because the missing key is the point. + const { ListId: _id, ...rest } = JSON.parse(body) as Record + expect(rest).toEqual({ + CreatorAccountId: 300, + Name: '__SavedForLater_Rooms', + Description: null, + ImageName: 'DefaultRoomImage.jpg', + Type: 1, + ItemIds: ['953'], + CreatedAt: expect.any(String), + }) + expect(Object.keys(rest)).not.toContain('Accessibility') + + // …and the client's own read finds it under the same three keys — and DOES carry the + // accessibility the save stored but did not echo. + expect(await readList(300)).toMatchObject({ ItemIds: ['953'], Accessibility: 0 }) +}) + +it('appends to the list it already created rather than making a second one', async () => { + const headers = await bearer('301') + const first = await (await saveForLater('__SavedForLater_Rooms', '953', headers)).text() + const second = await (await saveForLater('__SavedForLater_Rooms', '641', headers)).text() + + // Same list id both times: the name is the list's identity, so the second save must find + // the first list rather than create a rival the read can never resolve. + const idOf = (b: string) => /"ListId":(\d+),/.exec(b)?.[1] + expect(idOf(second)).toBe(idOf(first)) + expect(await readList(301)).toMatchObject({ ItemIds: ['953', '641'] }) +}) + +it('is idempotent — saving the same room twice shows it once', async () => { + const headers = await bearer('302') + await saveForLater('__SavedForLater_Rooms', '953', headers) + await saveForLater('__SavedForLater_Rooms', '641', headers) + const again = await saveForLater('__SavedForLater_Rooms', '953', headers) + expect(again.status).toBe(200) + + // Kept in the position it was FIRST saved into, not moved to the end. + expect(await readList(302)).toMatchObject({ ItemIds: ['953', '641'] }) +}) + +it('owns the list by the TOKEN, not by anything the caller can name', async () => { + await saveForLater('__SavedForLater_Rooms', '953', await bearer('303')) + await saveForLater('__SavedForLater_Rooms', '641', await bearer('304')) + + // Two players, two lists — neither can reach the other's, and the route takes no + // creatorAccountId at all, so there is nothing to write into someone else's list with. + expect((await readList(303)).ItemIds).toEqual(['953']) + expect((await readList(304)).ItemIds).toEqual(['641']) +}) + +it('refuses an unauthenticated save', async () => { + const res = await saveForLater('__SavedForLater_Rooms', '953', {}) + expect(res.status).toBe(401) +}) + +it('creates the list under the type the body names', async () => { + // `type` is part of a list's identity, so a body naming 4 must not land in the type-1 + // list the client reads back — and must be findable under 4. + await saveForLater('__SavedForLater_Items', '77', await bearer('305'), 'accessibility=0&type=4') + expect((await readList(305, 4, '__SavedForLater_Items')).ItemIds).toEqual(['77']) + expect((await readList(305, 1, '__SavedForLater_Items')).ItemIds).toEqual([]) +}) + +it('defaults a bodiless save to a private room list', async () => { + // A body that never arrives (or fails to parse) must not strand the list under a type the + // client's own read can't find. + const res = await SELF.fetch( + `${ORIGIN}/curatedlists/__SavedForLater_Rooms/items/953/createlistifneeded`, + { method: 'PUT', headers: await bearer('306') } + ) + expect(res.status).toBe(200) + expect(await readList(306)).toMatchObject({ Type: 1, Accessibility: 0, ItemIds: ['953'] }) +}) + +it('leaves an existing list’s accessibility alone on a later save', async () => { + const headers = await bearer('307') + await saveForLater('__SavedForLater_Rooms', '953', headers, 'accessibility=1&type=1') + expect((await readList(307)).Accessibility).toBe(1) + + // The client sends accessibility on every add, but this call is "add an item", not + // "change who can see the list" — one stray add must not flip a list the player made public. + await saveForLater('__SavedForLater_Rooms', '641', headers, 'accessibility=0&type=1') + expect(await readList(307)).toMatchObject({ Accessibility: 1, ItemIds: ['953', '641'] }) +}) + it('serves a discovery row from /algorithmiclists', async () => { const res = await SELF.fetch( `${ORIGIN}/algorithmiclists/Rooms_Battle_AlgoEndpoint_PlayHighlight_TabsTest_Explore?type=1` diff --git a/apps/lists/wrangler.jsonc b/apps/lists/wrangler.jsonc index 6d4017d..b7e0c46 100644 --- a/apps/lists/wrangler.jsonc +++ b/apps/lists/wrangler.jsonc @@ -4,15 +4,20 @@ "main": "src/lists.app.ts", "compatibility_date": "2026-06-16", "compatibility_flags": ["nodejs_compat"], - // Shared `recflare` DB (schema/migrations owned by the `rooms` worker). Read-only + // Shared `recflare` DB. The room tables (owned by the `rooms` worker) are read-only // here: `/algorithmiclists/HotList` ranks the same rooms the rooms worker's `/rooms/hot` - // feed does, rather than serving a canned list of ids. The "local" placeholder is - // replaced with the real id from RECFLARE_D1 at deploy time. + // feed does, rather than serving a canned list of ids. This worker owns the `list` / + // `list_item` tables behind the player-owned curated lists (schema/migration here); its + // own migrations_table keeps history separate from the other workers' migrations on the + // shared database. The "local" placeholder is replaced with the real id from RECFLARE_D1 + // at deploy time. "d1_databases": [ { "binding": "DB", "database_name": "recflare", - "database_id": "local" + "database_id": "local", + "migrations_dir": "migrations", + "migrations_table": "d1_migrations_lists" } ], // Shared Secrets Store holding the HS256 JWT signing key. Every worker binds the diff --git a/packages/domain/src/index.ts b/packages/domain/src/index.ts index ff2db19..7b38db3 100644 --- a/packages/domain/src/index.ts +++ b/packages/domain/src/index.ts @@ -8,6 +8,7 @@ export * from './room-instance-db' export * from './presence-db' export * from './gifts-db' export * from './inventory-invention-db' +export * from './lists-db' export * from './outfits-db' export * from './progression-db' export * from './relationships-db' diff --git a/packages/domain/src/lists-db.ts b/packages/domain/src/lists-db.ts new file mode 100644 index 0000000..ad5aa40 --- /dev/null +++ b/packages/domain/src/lists-db.ts @@ -0,0 +1,220 @@ +/** + * Player-owned curated lists on the shared `recflare` D1 database — the playlists a player + * builds themselves, of which "Saved for Later" is the one the client creates on its own. + * + * Columns rather than a JSON blob (the `club_announcement` pattern rather than the + * `room`/`club` one): nothing here is a client-shaped document that has to survive + * round-tripping, it is five scalars and a set of ids, and the ids need their own table to + * be queried and de-duplicated at all. + * + * A list is GENERIC. `list_type` says what the `item_id`s ARE — the `ListEntityType` the + * algorithmic lists echo, so `1` is Rooms and an `item_id` is a room id — and nothing here + * interprets them: the client resolves each id against the service that type names. That is + * also why `item_id` is TEXT rather than an integer. A list of rooms carries room ids, but a + * list of discovery sections carries section KEYS, and one column has to hold both. + * + * The `lists` worker owns this schema/migration (`apps/lists/migrations/0001_curated_list.sql`, + * applied under its own `migrations_table` so it doesn't clash with the other workers' + * migrations that share the database). `CURATED_LIST_SCHEMA_DDL` mirrors that migration so + * tests can build the tables directly. + */ + +/** Schema DDL (mirror of apps/lists/migrations/0001_curated_list.sql). */ +export const CURATED_LIST_SCHEMA_DDL: string[] = [ + // `list_id` is an ordinary autoincrement integer. The reference's own ids run to 18 + // digits (`624765592684307326`) and the static captures still carry theirs verbatim, but + // nothing requires a list this server MINTS to look like that — and a small id stays well + // inside what a JS number holds exactly, so it can't be rounded on its way through D1 or + // JSON. AUTOINCREMENT rather than a bare rowid alias: a list id is handed to the client, + // so a deleted list's id must not be handed out again to a different list. + `CREATE TABLE IF NOT EXISTS list ( + list_id INTEGER PRIMARY KEY AUTOINCREMENT, + creator_account_id INTEGER NOT NULL, + list_type INTEGER NOT NULL, + list_name TEXT NOT NULL, + list_name_lower TEXT GENERATED ALWAYS AS (lower(list_name)) VIRTUAL, + list_description TEXT, + image_name TEXT NOT NULL DEFAULT '', + accessibility INTEGER NOT NULL DEFAULT 1, + created_at TEXT NOT NULL + )`, + // The lookup the client actually makes: `?creatorAccountId=&type=&name=`, all three at + // once. UNIQUE because that triple is a list's identity — the client asks for + // `__SavedForLater_Rooms` by name expecting the one it has been appending to, so a + // player must never end up with two. Folded, since the casing that reaches us is the + // client's. + `CREATE UNIQUE INDEX IF NOT EXISTS idx_list_owner_type_name + ON list (creator_account_id, list_type, list_name_lower)`, + `CREATE INDEX IF NOT EXISTS idx_list_creator ON list (creator_account_id)`, + // A list's contents — one row per item, insertion order preserved by the surrogate key, + // which is the order the `ItemIds` array is served in. + // + // UNIQUE on the pair: saving the same room twice is a no-op, not a carousel showing it + // twice. The section's own `supportsDedupe` is about dedupe ACROSS rows and doesn't help + // here. + `CREATE TABLE IF NOT EXISTS list_item ( + list_item_id INTEGER PRIMARY KEY AUTOINCREMENT, + list_id INTEGER NOT NULL, + item_id TEXT NOT NULL + )`, + `CREATE UNIQUE INDEX IF NOT EXISTS idx_list_item_pair ON list_item (list_id, item_id)`, + `CREATE INDEX IF NOT EXISTS idx_list_item_list ON list_item (list_id)`, +] + +/** + * One curated list as the client parses it, whether it came out of D1 or out of a static + * capture. `Description` may be null but `ImageName` must be a STRING — the client reads it + * straight into a string field — and `ItemIds` are strings even where they stand for + * numeric ids, which is what the working captures carry. + * + * `ListId` is a string HERE ONLY and never reaches the client as one: the `lists` worker's + * `serializeCuratedList` puts the digits back on the wire unquoted, because the client's + * field is a number and a quoted id fails its parser. It stays a string even though a + * STORED id is a small integer, because a CAPTURED one is 18 digits — parsing that would + * round it (…307326 → …307300) — and both kinds flow through this one shape. + */ +export interface CuratedList { + ListId: string + CreatorAccountId: number + Name: string + Description: string | null + ImageName: string + Type: number + ItemIds: string[] + Accessibility?: number + CreatedAt: string +} + +interface ListRow { + list_id: number + creator_account_id: number + list_type: number + list_name: string + list_description: string | null + image_name: string + accessibility: number + created_at: string +} + +/** + * The image a list carries when nothing set one. Every captured list uses it, and the field + * cannot be empty or null without the client rendering a blank tile for the row. + */ +export const DEFAULT_LIST_IMAGE = 'DefaultRoomImage.jpg' + +/** A stored row plus its items, as the client-facing list. */ +function toCuratedList(row: ListRow, itemIds: string[]): CuratedList { + return { + ListId: String(row.list_id), + CreatorAccountId: row.creator_account_id, + Name: row.list_name, + Description: row.list_description, + ImageName: row.image_name, + Type: row.list_type, + ItemIds: itemIds, + Accessibility: row.accessibility, + CreatedAt: row.created_at, + } +} + +/** A list's item ids, in the order they were added — the order the row displays them. */ +async function getListItems(db: D1Database, listId: number): Promise { + const { results } = await db + .prepare('SELECT item_id FROM list_item WHERE list_id = ?1 ORDER BY list_item_id') + .bind(listId) + .all<{ item_id: string }>() + return results.map((r) => r.item_id) +} + +/** What identifies a player's list, and what a missing one is created with. */ +export interface PlayerListKey { + creatorAccountId: number + /** The `ListEntityType` — what the item ids ARE. 1 (Rooms) is what the client sends. */ + type: number + name: string + /** Applied only when the list is CREATED; see {@link addPlayerListItem}. */ + accessibility: number +} + +/** + * Add an item to a player's list, creating the list if they don't have one yet — the + * `…/items/:itemId/createlistifneeded` call the client makes when someone saves a room for + * later. Answers the list as it now stands, which is what the caller re-renders the row from. + * + * Idempotent in both halves. The list insert is `OR IGNORE` against the + * (creator, type, name) unique index and the id is re-read rather than assumed, so two adds + * racing to create the same list end up with ONE list and the loser adopts the winner's id + * instead of silently writing its item into a list nobody will look up. The item insert is + * `OR IGNORE` against (list_id, item_id), so saving the same room twice leaves the row + * showing it once, in the position it was first saved into. + * + * `accessibility` is honoured only on creation. The client sends it on every add, but this + * call is "add an item", not "change who can see the list" — applying it each time would let + * one stray add flip a list the player had deliberately made public, or the reverse. + */ +export async function addPlayerListItem( + db: D1Database, + key: PlayerListKey, + itemId: string +): Promise { + await db + .prepare( + `INSERT OR IGNORE INTO list (creator_account_id, list_type, list_name, + list_description, image_name, accessibility, created_at) + VALUES (?1, ?2, ?3, NULL, ?4, ?5, ?6)` + ) + .bind( + key.creatorAccountId, + key.type, + key.name, + DEFAULT_LIST_IMAGE, + key.accessibility, + new Date().toISOString() + ) + .run() + + // Read the id back rather than taking the insert's: `OR IGNORE` assigns nothing when the + // player already had the list, and says nothing about whether the row is ours. + const row = await db + .prepare( + `SELECT list_id FROM list + WHERE creator_account_id = ?1 AND list_type = ?2 AND list_name_lower = lower(?3)` + ) + .bind(key.creatorAccountId, key.type, key.name) + .first<{ list_id: number }>() + const listId = row!.list_id + + await db + .prepare('INSERT OR IGNORE INTO list_item (list_id, item_id) VALUES (?1, ?2)') + .bind(listId, itemId) + .run() + + return (await getPlayerList(db, key.creatorAccountId, key.type, key.name))! +} + +/** + * A player's own list, looked up the way the client asks for one: + * `?creatorAccountId=&type=&name=`. Undefined when that player has no such list — the + * caller decides what an absent list answers, since a static capture may cover the name. + * + * The name is matched case-insensitively; a list with no items is still a list and comes + * back with an empty `ItemIds`, which is NOT the same answer as undefined. + */ +export async function getPlayerList( + db: D1Database, + creatorAccountId: number, + type: number, + name: string +): Promise { + 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 creator_account_id = ?1 AND list_type = ?2 AND list_name_lower = lower(?3)` + ) + .bind(creatorAccountId, type, name) + .first() + if (row === null) return undefined + return toCuratedList(row, await getListItems(db, row.list_id)) +} diff --git a/packages/domain/src/rooms-db.ts b/packages/domain/src/rooms-db.ts index 3518794..5c2564b 100644 --- a/packages/domain/src/rooms-db.ts +++ b/packages/domain/src/rooms-db.ts @@ -891,21 +891,30 @@ interface RoomRow { const ROOM_COLUMNS = 'data, visits' /** - * Two keys on the client's room DTO that nothing here stores, defaulted on every read so - * the key is PRESENT rather than absent — the seed blobs and every room written since - * predate them, so they can't come from the data: + * Keys on the client's room DTO that nothing here stores, defaulted on every read so the + * key is PRESENT rather than absent — the seed blobs and every room written since predate + * them, so they can't come from the data: * * - `BoostCount` — how many boosts the room is carrying. No boost feature exists here, so * it is 0 for every room. * - `CurrentSnapshotId` — the room's published snapshot. Nothing takes snapshots, so it is * null, which is also what the reference serves for a room that has none. + * - `FriendlyName` — the display name, which the reference lets a creator set apart from + * the unique `Name`. Nothing sets one here, so it falls back to `Name`; it must never be + * null, because the client labels a room from it and renders nothing for a room without + * one. + * - `CCU` — concurrent users. No live-population counter exists here, so it is null, which + * is what the reference serves when it has no number rather than 0 (a 0 reads as "nobody + * is in here" in the browse feeds). * - * Defaulted rather than assigned, so a stored value wins if either is ever really written + * Defaulted rather than assigned, so a stored value wins if any is ever really written * (a blob keeps whatever `serializeRoom` last put in it). */ function attachRoomDtoDefaults(room: Room): void { room.BoostCount ??= 0 room.CurrentSnapshotId ??= null + room.FriendlyName ??= room.Name + room.CCU ??= null } /** @@ -1782,33 +1791,39 @@ export async function countRoomsByCreator(db: D1Database, accountId: number): Pr } /** - * Rooms an account CONTRIBUTES to: the ones whose `Roles` name them (Host, Moderator or - * CoOwner), minus the ones they created themselves. + * Every room an account works on: the ones it CREATED plus the ones whose `Roles` name it + * (Host, Moderator or CoOwner). Every role tier counts, unlike {@link canManageRoom}'s + * owner-or-co-owner gate: this is "you have a job in this room", not "you may administer + * it". * - * The creator is excluded deliberately. A room's `Roles` carries its creator too, so - * without that filter this list would repeat everything `getRoomsByCreator` already - * serves — and the client shows "rooms you own" and "rooms you contribute to" as two - * separate lists. Every role tier counts here, unlike {@link canManageRoom}'s - * owner-or-co-owner gate: this is "somebody gave you a job in their room", not "you may - * administer it". + * The creator half used to be excluded — a room's `Roles` carries its creator too, and the + * client shows "rooms you own" and "rooms you contribute to" as separate lists, so the + * exclusion kept this from repeating `createdby/me`. It also made the list EMPTY for every + * account that had only ever built its own rooms, which is most of them, so the screen + * behind it showed nothing at all. Repeating `createdby/me` is the better failure, and + * overlap is what a client that renders one list wants anyway. + * + * The dorm stays out, on `ownedby/me`'s reasoning: it is auto-provisioned rather than a + * room the player made. A room matching BOTH halves appears once — the roles half is an + * EXISTS, not a join. * * Roles live inside the room blob rather than in a table of their own, so the match is a * `json_each` over `$.Roles`. A room with no `Roles` key (or a null one) simply yields no - * rows there rather than erroring, so it drops out of the list. + * rows there rather than erroring, so it only reaches the list if the account created it. */ export async function getContributedRooms(db: D1Database, accountId: number): Promise { const { results } = await db .prepare( `SELECT ${ROOM_COLUMNS} FROM room - WHERE creator_account_id IS NOT ?1 - AND EXISTS ( + WHERE creator_account_id = ?1 + OR EXISTS ( SELECT 1 FROM json_each(room.data, '$.Roles') AS role WHERE json_extract(role.value, '$.AccountId') = ?1 )` ) .bind(accountId) .all() - return hydrateRooms(db, parseAll(results)) + return (await hydrateRooms(db, parseAll(results))).filter((r) => r.IsDorm !== true) } /** @@ -2006,6 +2021,8 @@ function roomHasAnyTag(room: Room, tags: Set): boolean { * `#tag` terms match the room's Tags; plain terms match the room name * (substring). All terms must match. Returns a paginated `{ Results, TotalResults }`. * The dataset is small, so this filters in memory rather than in SQL. + * + * `#community` is the one tag term that isn't a tag lookup — see {@link COMMUNITY_TAG}. */ export async function searchRooms( db: D1Database, @@ -2021,9 +2038,15 @@ export async function searchRooms( // EVERY tag asked for, and each term expands to its aliases (`#recroomoriginal` accepts // `rro`). Only the rooms that survive have their blobs read, which is what `room_tag` is // for: a tag search no longer parses every room in the database to ask. - const tagSets = terms - .filter((t) => t.startsWith('#')) - .map((t) => t.slice(1)) + // + // `#community` is held out of that query: no room CARRIES the tag (the browse chip posts + // it to the hot feed as a pseudo-tag, and the search box sends the same term), so asking + // `room_tag` for it matches nothing and the whole search comes back empty. It filters on + // who MADE the room instead, below. + const tagTerms = terms.filter((t) => t.startsWith('#')).map((t) => t.slice(1)) + const communityOnly = tagTerms.includes(COMMUNITY_TAG) + const tagSets = tagTerms + .filter((tag) => tag !== COMMUNITY_TAG) .map((tag) => [tag, ...(TAG_ALIASES[tag] ?? [])]) const { sql, binds } = roomsByTagsQuery(tagSets) const { results } = await db @@ -2032,6 +2055,11 @@ export async function searchRooms( .all() let rooms = parseAll(results).filter((r) => r.IsDorm !== true && r.Accessibility === 1) + // The same test the hot feed's `community` chip applies: every room a player made, which + // is every room the Coach account doesn't own. It narrows the other terms rather than + // replacing them, so `#community horror` is still a name search within player-made rooms. + if (communityOnly) rooms = rooms.filter(isPlayerMade) + // The plain terms still match in memory: they are substring matches on the name, which // no index helps with. for (const term of terms) { @@ -2135,6 +2163,9 @@ const NEW_TAG = 'new' * isn't the Coach account — the system account that owns the seeded Rec Room rooms. * Unlike {@link NEW_TAG} it only filters: the page keeps the feed's normal * live-population ordering. + * + * The chip reaches {@link searchRooms} too, as the tag term `#community` — the search box + * carries the same word — so both feeds have to know it names no tag. */ const COMMUNITY_TAG = 'community'