From 55ac92670f59634912cc06dd498b7c21dea85426 Mon Sep 17 00:00:00 2001 From: Devin Zuczek Date: Mon, 24 Aug 2026 18:17:17 -0400 Subject: [PATCH] [lists] more lists testing --- apps/lists/src/curated-lists.ts | 42 ++++---- apps/lists/src/lists.app.ts | 102 ++++++++++++++++---- apps/lists/src/test/integration/api.test.ts | 94 +++++++++++------- 3 files changed, 159 insertions(+), 79 deletions(-) diff --git a/apps/lists/src/curated-lists.ts b/apps/lists/src/curated-lists.ts index 661af89..294b606 100644 --- a/apps/lists/src/curated-lists.ts +++ b/apps/lists/src/curated-lists.ts @@ -79,9 +79,10 @@ const BY_TYPE_NAME = new Map() /** By name alone, for a name whose `type` doesn't line up with what it is captured under. */ const BY_NAME = new Map() /** - * By type alone: the page's DEFAULT list, which is what answers a `name` this server has - * nothing under — the store's Featured page, for one, asks for its rows by the reference's - * numeric list id (`name=17859340`) rather than by a name. + * By type alone: the page's DEFAULT list, which answers a request that names NO list — only + * a request that names none. A request that DOES name one and matches nothing is a miss and + * 404s; handing it the page default answers a question nobody asked, under a heading that + * belongs to another list. */ const BY_TYPE = new Map() @@ -95,16 +96,6 @@ for (const list of CURATED_LISTS) { if (!BY_TYPE.has(list.Type)) BY_TYPE.set(list.Type, list) } -/** - * What a request that matches nothing at all gets: the Play menu's Explore panel. An empty - * body — or a 404 — renders as a blank page rather than an error, so answering with - * SOMETHING is the better failure, and it is what the reference was observed doing for a - * list it had nothing under. Falls back to whatever sits first if that capture is ever - * dropped from the array. - */ -const DEFAULT_CURATED_LIST = - BY_NAME.get(nameKey('Discovery.PageSource.PlayExplore')) ?? CURATED_LISTS[0] - /** * 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 @@ -140,28 +131,30 @@ function emptyReservedList(creatorAccountId: string | undefined, type: number, n /** * 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. + * and had nothing — the static captures, resolved most-specific first. UNDEFINED when + * nothing matches, which the route turns into a 404: a name this server has nothing under + * is a list that does not exist, and answering it with an unrelated capture puts one page's + * rows under another page's heading. * * `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. + * Two things still answer without a name match: * - * 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. + * - A RESERVED name (`__SavedForLater_Rooms`), which comes back EMPTY rather than missing: + * it is a list the client creates for itself, so "the player has saved nothing" is the + * right answer until they do, and an empty list hides the row the way its + * `minItemsToShowSection` asks for. + * - A request naming no list at all, which gets the page default for its type — the only + * list it could be asking for. */ export function resolveCuratedList( creatorAccountId: string | undefined, type: string | undefined, name: string | undefined -): CuratedList { +): CuratedList | undefined { const key = nameKey(name ?? '') const parsedType = Number.parseInt(type ?? '', 10) const hasType = Number.isInteger(parsedType) @@ -173,7 +166,6 @@ export function resolveCuratedList( (isReservedListName(name) ? emptyReservedList(creatorAccountId, hasType ? parsedType : 0, name ?? '') : undefined) ?? - (hasType ? BY_TYPE.get(parsedType) : undefined) ?? - DEFAULT_CURATED_LIST + (key === '' && hasType ? BY_TYPE.get(parsedType) : undefined) ) } diff --git a/apps/lists/src/lists.app.ts b/apps/lists/src/lists.app.ts index 74bc153..50a93bc 100644 --- a/apps/lists/src/lists.app.ts +++ b/apps/lists/src/lists.app.ts @@ -59,17 +59,31 @@ const ListEntityType = { const MAX_LIST_ENTITY_TYPE = 255 /** - * The fallback entities for a row with no live feed behind it (`GET /algorithmiclists/:list`) - * — ROOMS, which is what a Play/Explore row is built from. Rooms 2–6, the low ids this - * server's own rooms occupy, so an unranked row resolves to something real instead of five - * dead ids. The rows in `ROW_FEEDS` are ranked for real and never reach this. - * - * `Id` is a STRING even though a room id is a number, and `Context` is where the reference - * server attributes the ranking/experiment that produced the entity. Nothing produced these, - * so it is null on every one rather than a made-up context the client would carry into - * telemetry. + * One entity of an algorithmic list. `Id` is a STRING even though most of the things a row + * names (rooms, items) are numbered, and `Context` is where the reference server attributes + * the ranking or experiment that produced the entity — nothing here produces one, so it is + * null on every entity rather than a made-up context the client would carry into telemetry. */ -const ALGORITHMIC_LIST_ENTITIES: Array<{ Id: string; Context: string | null }> = [].map((Id) => ({ Id, Context: null })) +interface ListEntity { + Id: string + Context: string | null +} + +/** Ids as the entities the client reads back — see `ListEntity`. */ +function entities(ids: string[]): ListEntity[] { + return ids.map((Id) => ({ Id, Context: null })) +} + +/** + * What a row with nothing behind it answers (`GET /algorithmiclists/:list`): NO entities. + * The row still answers 200 — a 404 renders as a carousel that failed to load rather than + * one the client hides — but it names nothing, because there is no honest answer for a row + * this server has never heard of. It once served rooms 2–6 so an unranked row resolved to + * something real; that put the same five stock rooms under every unimplemented heading, + * which reads as a broken row rather than an absent one. The rows with a real answer are in + * `ROW_FEEDS`, `PERSONAL_ROW_FEEDS` and `STATIC_ROW_ENTITIES`. + */ +const ALGORITHMIC_LIST_ENTITIES: ListEntity[] = [] /** How many rooms a row carries. A discovery carousel shows a page, not the world. */ const LIST_SIZE = 20 @@ -77,10 +91,10 @@ const LIST_SIZE = 20 /** * A row's rooms as the entities the client reads back. Only the ids travel — it resolves * each room against the `rooms` worker itself — so `Id` is the room id as a STRING and - * `Context` (the ranking attribution) is null, exactly as in `ALGORITHMIC_LIST_ENTITIES`. + * `Context` (the ranking attribution) is null, like every other entity. */ -function toEntities(rooms: Room[]): Array<{ Id: string; Context: string | null }> { - return rooms.map((room) => ({ Id: String(room.RoomId), Context: null })) +function toEntities(rooms: Room[]): ListEntity[] { + return entities(rooms.map((room) => String(room.RoomId))) } /** @@ -183,6 +197,43 @@ const PERSONAL_ROW_FEEDS: Record recentlyvisited: (db, accountId) => getVisitedRooms(db, accountId, 0, LIST_SIZE), } +/** + * The placeholder contents of a STORE carousel: four purchasable items out of storefront 3, + * held here because nothing on this server ranks store items yet and what the reference + * actually served these rows is not known. Every store row shares the one list rather than + * each carrying its own copy — they are all the same placeholder, and a row that gets a real + * answer should stop pointing at it rather than have its ids edited in place. + */ +const STORE_PLACEHOLDER_ITEMS = entities(['257', '192', '641', '657']) + +/** + * Rows served from a FIXED id list — a carousel somebody picked by hand, with no ranking + * behind it. Keyed and looked up exactly like `ROW_FEEDS` (lowercase, folded), and checked + * after it, so a row that later grows a real feed is promoted by moving its line up there. + * + * Unlike the room rows, these do NOT all name rooms: the store's carousels name purchasable + * items, which is why the ids are written out as the strings they go on the wire as rather + * than derived from anything. The `Type` the response reports is still the caller's — see + * the handler. + * + * The slugs are the reference's own and several do not match the heading they render under + * (`summerpartycarousel` fills a medieval row). Key on the SLUG regardless: it is what the + * discovery section's `sourceMetadata` names, so renaming one to something that reads better + * would leave that section pointing at nothing. + */ +const STATIC_ROW_ENTITIES: Record = { + // The store Featured page's "Medieval Masterpieces from the Community" carousel — + // `StoreItemCarousel_UnifiedAlgorithmicList_UGCMedievalCarousel` in the `discovery` + // worker's `StoreFeatured` page. Asked for with `?type=5`, Generic. + summerpartycarousel: STORE_PLACEHOLDER_ITEMS, + + // The store Clothing page's "New" carousel — + // `StoreItemCarousel_UnifiedAlgorithmicList_New` in `StoreClothing`, and the `newitems` + // category the client's own store-category game config lists. Same placeholder items as + // the row above until something here actually knows which items are new. + newitems: STORE_PLACEHOLDER_ITEMS, +} + /** * The entity type an algorithmic list reports when the query names none. The client always * sends `?type=`, and `Rooms` is what it asks for; falling back to `Accounts` (0, the enum's @@ -292,8 +343,13 @@ const app = new Hono() // // 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`). + // Nothing else distinguishes the two requests: both are the same three parameters. + // + // A name that matches NEITHER 404s. There is no list to serve, and the fallbacks this + // once had answered with an unrelated capture instead — a page's rows under another + // page's heading, which reads as real content rather than as a missing list. The + // exceptions are the client's own reserved playlists and a request naming no list at all; + // both are real answers, not misses (see `resolveCuratedList`). .get('/curatedlists', async (c) => { const creatorAccountId = c.req.query('creatorAccountId') const type = c.req.query('type') @@ -302,6 +358,7 @@ const app = new Hono() const list = (await ownedList(c, creatorAccountId, type, name)) ?? resolveCuratedList(creatorAccountId, type, name) + if (list === undefined) return c.notFound() // 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 @@ -358,10 +415,11 @@ const app = new Hono() // `Rooms_Battle_AlgoEndpoint_PlayHighlight_TabsTest_Explore`), and the answer is the // ranked entities that fill it, which the client then resolves by id itself. // - // `HotList`, `recentlyupdated` and `new` are ranked for real (see `ROW_FEEDS`), and - // `recentlyvisited` is per-caller (see `PERSONAL_ROW_FEEDS`). Every other row still serves - // the canned entities, and an unknown row key gets them too rather than a 404, which the - // client renders as a row that failed to load. `Type` is echoed back from the query: it + // `HotList`, `recentlyupdated` and `new` are ranked for real (see `ROW_FEEDS`), + // `recentlyvisited` is per-caller (see `PERSONAL_ROW_FEEDS`), and a few are hand-picked id + // lists (see `STATIC_ROW_ENTITIES`). 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. `Type` is echoed back from the query: it // tells the client what the `Id`s ARE (rooms, players, …), so answering with a type the // caller didn't ask for would have it resolve the ids against the wrong service. .get('/algorithmiclists/:list', async (c) => { @@ -393,6 +451,12 @@ const app = new Hono() return c.json({ Type: echoed, Entities: toEntities(await feed(c.env.DB)) }) } + // Then the hand-picked rows, which are already entities: the ids are the answer. + const canned = STATIC_ROW_ENTITIES[key] + if (canned !== undefined) { + return c.json({ Type: echoed, Entities: canned }) + } + return c.json({ Type: echoed, Entities: ALGORITHMIC_LIST_ENTITIES }) }) diff --git a/apps/lists/src/test/integration/api.test.ts b/apps/lists/src/test/integration/api.test.ts index c3abb5a..d3489eb 100644 --- a/apps/lists/src/test/integration/api.test.ts +++ b/apps/lists/src/test/integration/api.test.ts @@ -294,24 +294,30 @@ it('matches the name case-insensitively and prefers it over the type', async () expect(((await explore.json()) as { Name: string }).Name).toBe('Discovery.PageSource.PlayExplore') }) -it('falls back to the default list for a name it has nothing under', async () => { - // The store's Featured page asks for its rows by the reference's numeric list id rather - // than by a name — nothing here is called that, and nothing is captured under its type - // either, so the default list answers. An empty body or a 404 would render as a blank - // page rather than an error. - const explore = await ( - await SELF.fetch(`${ORIGIN}/curatedlists?name=Discovery.PageSource.PlayExplore`) - ).text() - +it('404s for a name it has nothing under', async () => { + // A name nothing matches is a list that does not exist. It used to answer with the + // default capture, which put one page's rows under another page's heading — content that + // looks real, where a 404 says plainly there is no such list. `17859340` is the store + // Featured page's own lookup (by the reference's numeric list id) and gets the same + // answer: nothing here is called that. for (const query of [ + '?creatorAccountId=1&type=5&name=Internal_Medieval_Items', '?creatorAccountId=1&type=4&name=17859340', '?type=99&name=Nope', - '?creatorAccountId=7&type=&name=', - '', + '?type=7&name=Discovery.PageSource.NotAPage', ]) { - const bare = await SELF.fetch(`${ORIGIN}/curatedlists${query}`) - expect(bare.status).toBe(200) - expect(await bare.text()).toBe(explore) + const res = await SELF.fetch(`${ORIGIN}/curatedlists${query}`) + expect(res.status).toBe(404) + } + + // Naming NO list is not a miss — it asks for the page default, and only the type says + // which page. Without a type there is no page either, so that 404s too. + const byType = await SELF.fetch(`${ORIGIN}/curatedlists?creatorAccountId=1&type=7`) + expect(byType.status).toBe(200) + expect(((await byType.json()) as { Name: string }).Name).toBe('Discovery.PageSource.PlayExplore') + + for (const query of ['?creatorAccountId=7&type=&name=', '']) { + expect((await SELF.fetch(`${ORIGIN}/curatedlists${query}`)).status).toBe(404) } }) @@ -451,16 +457,14 @@ it('answers an unowned reserved list EMPTY rather than with the default page', a 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() +it('404s for a NON-reserved unknown name', async () => { + // The empty-list answer is the reserved prefix's alone: a name a player did not reserve + // and this server has nothing under is missing, not empty. An empty list would have the + // client render a real but blank row for a page that does not exist here. const featured = await SELF.fetch( `${ORIGIN}/curatedlists?creatorAccountId=1&type=4&name=17859340` ) - expect(await featured.text()).toBe(explore) + expect(featured.status).toBe(404) }) it('prefers a player’s stored list over a capture of the same name', async () => { @@ -613,19 +617,39 @@ it('serves a discovery row from /algorithmiclists', async () => { ) expect(res.status).toBe(200) // `Type` is echoed from the query — it says what the ids ARE (1 = rooms), so the client - // resolves them against the right service. Ids are STRINGS even though a room id is a - // number, and `Context` (the ranking attribution) is null: nothing ranks anything here - // yet, so every row serves rooms 2–6. - expect(await res.json()).toEqual({ - Type: 1, + // resolves them against the right service. Nothing ranks this row, so it answers 200 with + // NO entities: the client hides an empty carousel, where a 404 would show it as one that + // failed to load. + expect(await res.json()).toEqual({ Type: 1, Entities: [] }) +}) + +it('serves the hand-picked summerpartycarousel row', async () => { + // The store's "Medieval Masterpieces from the Community" carousel, which the client asks + // for by the section's `sourceMetadata` slug and with `?type=5` (Generic). Nothing ranks + // store items here, so the row is a fixed id list — same entity shape as any other row, + // ids as STRINGS and `Context` null. + const res = await SELF.fetch(`${ORIGIN}/algorithmiclists/summerpartycarousel?type=5`) + expect(res.status).toBe(200) + const expected = { + Type: 5, Entities: [ - { Id: '2', Context: null }, - { Id: '3', Context: null }, - { Id: '4', Context: null }, - { Id: '5', Context: null }, - { Id: '6', Context: null }, + { Id: '257', Context: null }, + { Id: '192', Context: null }, + { Id: '641', Context: null }, + { Id: '657', Context: null }, ], - }) + } + expect(await res.json()).toEqual(expected) + + // Looked up folded, like every other row key: the casing is the reference's, not ours. + const cased = await SELF.fetch(`${ORIGIN}/algorithmiclists/SummerPartyCarousel?type=5`) + expect(await cased.json()).toEqual(expected) + + // The store Clothing page's "New" carousel serves the same placeholder items — both are + // store rows nothing ranks yet, so they share one id list rather than drifting apart. + const newItems = await SELF.fetch(`${ORIGIN}/algorithmiclists/newitems?type=5`) + expect(newItems.status).toBe(200) + expect(await newItems.json()).toEqual(expected) }) it('serves the live hot-room ranking for /algorithmiclists/HotList', async () => { @@ -827,10 +851,10 @@ it('echoes the requested type and answers an unknown row', async () => { const other = await SELF.fetch(`${ORIGIN}/algorithmiclists/Nothing_Ranks_This_Row?type=4`) expect(other.status).toBe(200) const body = (await other.json()) as { Type: number; Entities: unknown[] } - // An unknown row key still gets the canned entities: a 404 renders as a row that failed - // to load rather than an empty one. + // An unknown row key answers 200 with no entities rather than 404ing: a failed request + // renders as a row that failed to load, an empty one as a row the client hides. expect(body.Type).toBe(4) - expect(body.Entities).toHaveLength(5) + expect(body.Entities).toEqual([]) // No `type` at all falls back to Rooms (1), the only one the client asks for — falling // back to the enum's zero value would have the row resolve room ids as ACCOUNTS.