From aeff7d50cdabd4b1d89ef2602e4520a9f97ecdd7 Mon Sep 17 00:00:00 2001 From: Devin Zuczek Date: Thu, 6 Aug 2026 14:47:52 -0400 Subject: [PATCH] fix issue with subroom save list not working in room with no current save --- CLAUDE.md | 21 ++++++--- apps/rooms/src/openapi.ts | 11 ++--- apps/rooms/src/test/integration/api.test.ts | 32 +++++++++----- packages/domain/src/rooms-db.ts | 49 ++++++++++++++++----- 4 files changed, 78 insertions(+), 35 deletions(-) diff --git a/CLAUDE.md b/CLAUDE.md index 334318c..f7434f0 100644 --- a/CLAUDE.md +++ b/CLAUDE.md @@ -90,20 +90,29 @@ inconsistency here without checking the client first. no asset arrays; but `unityAsset`/`unityAssetHash`). Don't unify the two projections. - A subroom's saved scene loads from `CurrentSave.DataBlob` (`rooms`: `GET /rooms/:id`), NOT the flat `DataBlob` on the subroom — a subroom with no `CurrentSave` silently loads - nothing. The key must be present (null before the first publish); read it via - `subRoomDataBlob()` so `match`/`auth` instance payloads resolve it the same way. + nothing. Read it via `subRoomDataBlob()` so `match`/`auth` instance payloads resolve it + the same way. It resolves in `attachCurrentSaves`, which every room read goes through: + the PUBLISHED save (`current_save_id`), falling back to the STAGED one + (`staged_save_id`) when nothing has been published yet, and with NEITHER the key is + omitted entirely — a `CurrentSave: null` breaks the client on every endpoint that serves + rooms (not just the save flow — `/rooms/ownedby/me` and friends too). Don't reintroduce + a null in a hand-built subroom payload. - A room save (`rooms`: `POST …/subrooms/:sid/data`) publishes only when the body says `AutoPublish: true`; otherwise it STAGES onto `StagedSubRoomDataSaveId` and leaves - `CurrentSave` alone, so players keep loading the last published version until the owner - posts `…/subrooms/:sid/publish_save` with `subRoomDataSaveId=`. DORMS always + `current_save_id` alone, so players keep loading the last published version until the + owner posts `…/subrooms/:sid/publish_save` with `subRoomDataSaveId=` — except before + the FIRST publish, where `CurrentSave` falls back to the staged save (there is no older + version to keep serving, and a subroom that has been saved shouldn't read as empty). + DORMS always publish: no publish step exists in the client for them. Saves live in the `subroom_save` table with globally-unique ids (a bare id has to resolve — `StagedSubRoomDataSaveId` carries no subroom context), and nothing is overwritten, so `…/saves` is real history and `publish_save` doubles as restore-a-save. `…/saves` is auth-gated and CREATOR-only (not co-owners) — it lists unpublished staged saves. There is no `GET …/subrooms/:sid/data`; only the POST (the room save) exists on that path. -- Matchmaking (`match`: `/matchmake/room/:roomId/:subRoomId`) always serves the PUBLISHED - `CurrentSave` blob, creator included. Joining a private instance, the client itself asks +- Matchmaking (`match`: `/matchmake/room/:roomId/:subRoomId`) serves whatever + `CurrentSave` resolves to — the published save once there is one — to everyone alike, + creator included. Joining a private instance, the client itself asks the owner whether to load the latest or the published version and resolves it from the `/subrooms/:sid/saves` list — the matchmake call is identical either way. Don't make this server-side: it would put two people in one instance on different versions. diff --git a/apps/rooms/src/openapi.ts b/apps/rooms/src/openapi.ts index 5bbcf79..a3e38da 100644 --- a/apps/rooms/src/openapi.ts +++ b/apps/rooms/src/openapi.ts @@ -213,9 +213,10 @@ export const SubRoomDataSaveDto = z.object({ * sequence, not per-room); a room's `SubRooms` array is reconstructed on read. * * `CreatorAccountId` starts null on the seeded rooms and is filled in on the first save — - * the client NREs on a null one. `CurrentSave` is null until the first save; the flat - * `DataBlob`/`RoomDataBlob`/`DataSavedAt` fields are legacy and are NOT what the client - * loads from. + * the client NREs on a null one. `CurrentSave` is the published save, or the staged one + * when nothing has been published; with neither it is ABSENT rather than null (a null one + * breaks the client's parser). The flat `DataBlob`/`RoomDataBlob`/`DataSavedAt` fields are + * legacy and are NOT what the client loads from. */ export const SubRoomDto = z.object({ SubRoomId: z.int(), @@ -231,8 +232,8 @@ export const SubRoomDto = z.object({ .describe('0 Private, 1 Public, 2 Unlisted, 3 Dev_only, 4 Dev_Unlisted — set independently'), ShouldAutoStageSaves: z.boolean(), StagedSubRoomDataSaveId: z.int().nullable(), - CurrentSave: SubRoomDataSaveDto.nullable().describe( - 'The latest room save — where the client finds the scene blob. Null until first save' + CurrentSave: SubRoomDataSaveDto.optional().describe( + 'The room save the client loads the scene blob from: the published one, falling back to the STAGED one when nothing has been published yet. Omitted entirely (never null) when the subroom has neither' ), DataBlob: z.string().optional().describe('Legacy flat key; the client reads `CurrentSave`'), RoomDataBlob: z.string().optional().describe('Uploaded room-data key; absent until first save'), diff --git a/apps/rooms/src/test/integration/api.test.ts b/apps/rooms/src/test/integration/api.test.ts index 953a7a6..e012491 100644 --- a/apps/rooms/src/test/integration/api.test.ts +++ b/apps/rooms/src/test/integration/api.test.ts @@ -1470,19 +1470,22 @@ describe('rooms endpoints', () => { expect(coOwnerEnv.value.subRoomDataSave).toMatchObject({ savedByAccountId: 2 }) }) - it('GET /rooms/:id gives every subroom a CurrentSave key (null before the first save)', async () => { - // The client loads a subroom's scene data from CurrentSave and nothing else, so - // the key must be PRESENT — the seeded rooms predate it and have no such field in - // their stored blob. `in` rather than a value check: absent and null differ here. + it('GET /rooms/:id omits CurrentSave entirely on a never-saved subroom', async () => { + // A null CurrentSave breaks the client's room parser, so an unsaved subroom has to + // carry no such key at all. `in` rather than a value check: absent and null differ + // here, and null is exactly what this must not be. // Room 3 is seeded and never saved by another test (room 2 is the save fixture). const room = (await (await SELF.fetch(`${ORIGIN}/rooms/3`)).json()) as { SubRooms: Array> } expect(room.SubRooms.length).toBeGreaterThan(0) for (const sub of room.SubRooms) { - expect('CurrentSave' in sub).toBe(true) - expect(sub.CurrentSave).toBeNull() + expect('CurrentSave' in sub).toBe(false) } + // Not just missing from the parsed object — absent from the wire bytes too, which is + // what the client's parser actually reads. + const raw = await (await SELF.fetch(`${ORIGIN}/rooms/3`)).text() + expect(raw).not.toContain('"CurrentSave"') }) it('a real client room-save body stages, and publish_save makes it live', async () => { @@ -1493,7 +1496,7 @@ describe('rooms endpoints', () => { body: JSON.stringify(body), }) type Sub = { - CurrentSave: Record | null + CurrentSave?: Record StagedSubRoomDataSaveId: number | null } const subOf = async () => (await subRoomOf(5, 5)) as unknown as Sub @@ -1520,11 +1523,16 @@ describe('rooms endpoints', () => { }) expect(res.status).toBe(200) - // Room 5 is not a dorm → staged, nothing live yet. + // Room 5 is not a dorm → the save is STAGED, not published. Nothing has ever been + // published here, so there is no older version to keep serving: CurrentSave falls + // back to the staged save rather than leaving the subroom reading as never-saved. const stagedSub = await subOf() - expect(stagedSub.CurrentSave).toBeNull() const firstId = stagedSub.StagedSubRoomDataSaveId! expect(firstId).toBeGreaterThan(0) + expect(stagedSub.CurrentSave).toMatchObject({ + SubRoomDataSaveId: firstId, + DataBlob: '2026-07-28/f176fc3b-scene', + }) // Publishing makes it what the loader fetches, and clears the staging slot. expect((await publish(firstId)).status).toBe(200) @@ -1548,7 +1556,7 @@ describe('rooms endpoints', () => { // It's on the room read too — that's what the loader actually fetches. const room = (await (await SELF.fetch(`${ORIGIN}/rooms/5`)).json()) as { - SubRooms: Array<{ SubRoomId: number; CurrentSave: { SubRoomDataSaveId: number } | null }> + SubRooms: Array<{ SubRoomId: number; CurrentSave?: { SubRoomDataSaveId: number } }> } expect(room.SubRooms.find((s) => s.SubRoomId === 5)!.CurrentSave!.SubRoomDataSaveId).toBe( firstId @@ -1693,9 +1701,9 @@ describe('rooms endpoints', () => { headers: await bearer('1'), }) const body = (await res.json()) as { - value: { SubRooms: Array<{ SubRoomId: number; CurrentSave: { SubRoomId: number } | null }> } + value: { SubRooms: Array<{ SubRoomId: number; CurrentSave?: { SubRoomId: number } }> } } - const clone = body.value.SubRooms.find((s) => s.SubRoomId !== 2 && s.CurrentSave !== null)! + const clone = body.value.SubRooms.find((s) => s.SubRoomId !== 2 && s.CurrentSave !== undefined)! expect(clone).toBeDefined() // The copy's save must claim the COPY, or the client resolves it against the source. expect(clone.CurrentSave!.SubRoomId).toBe(clone.SubRoomId) diff --git a/packages/domain/src/rooms-db.ts b/packages/domain/src/rooms-db.ts index c78d8e1..6b3d771 100644 --- a/packages/domain/src/rooms-db.ts +++ b/packages/domain/src/rooms-db.ts @@ -797,7 +797,7 @@ export async function createSubRoom( LastModeratedSaveModerationState: 0, ShouldAutoStageSaves: true, // Nothing saved yet — the first room save mints one and points current_save_id - // at it. Until then the subroom reads with `CurrentSave: null`. + // at it. Until then the subroom reads with no `CurrentSave` key at all. }) // Refresh the hydrated SubRooms so the returned room includes the one just inserted. await attachSubRooms(db, [room]) @@ -863,10 +863,11 @@ const SUBROOM_COLUMNS = 'sub_room_id, room_id, data, current_save_id, staged_sav /** * Materialize a subroom row into its client shape, with the columns authoritative. - * `CurrentSave` is left undefined here and filled in by {@link attachCurrentSaves} — it - * lives in `subroom_save`, and resolving it per row would be a query each. Callers must - * go through the helpers below so the key is never missing: the client reads the scene - * blob from `CurrentSave` and nowhere else, so a subroom without one loads nothing. + * `CurrentSave` is left alone here and resolved by {@link attachCurrentSaves} — it lives + * in `subroom_save`, and resolving it per row would be a query each. Callers must go + * through the helpers below rather than this: the client reads the scene blob from + * `CurrentSave` and nowhere else, so a subroom served without one loads nothing, and a + * stale one spread out of the stored blob would point at another subroom's save. */ const parseSubRoomRow = (row: SubRoomRow): SubRoom => ({ ...(JSON.parse(row.data) as SubRoom), @@ -903,12 +904,31 @@ const serializeRoom = (room: Room): string => { return JSON.stringify({ ...rest, Stats: storedStats(stats) }) } +/** + * The save a subroom actually serves as its `CurrentSave`: the published one, or the + * staged one when nothing has been published yet. Null when it has neither. + */ +const liveSaveId = (row: SubRoomRow): number | null => row.current_save_id ?? row.staged_save_id + /** * Fill in each subroom's `CurrentSave` from `subroom_save`, in ONE query for the whole - * batch. Every subroom ends up with the key present — null when it points at no save - * (never saved) or the pointer dangles — because the client's loader reads it directly. + * batch. * - * `rows` must line up with `subs` positionally; the pointer lives on the row, not the + * `current_save_id` (the published save) wins; a subroom that has none FALLS BACK to + * `staged_save_id`, so a subroom whose only save is unpublished still serves that save as + * its `CurrentSave` rather than reading as never-saved. Note what this means for staging: + * before the first publish there is nothing older to keep serving, so the staged work is + * what everyone loads — but once a save has been published, staging behaves as it did and + * players keep loading the published version. + * + * A subroom with neither pointer (never saved, or both dangling) has the key REMOVED + * rather than set to null: a null `CurrentSave` breaks the client's room parser, so an + * unsaved subroom has to look like it has no such field at all. The key is deleted rather + * than merely left unset because the stored blob of a subroom written before saves moved + * to their own table can still carry a `CurrentSave: null` of its own, which + * {@link parseSubRoomRow} would spread straight through. + * + * `rows` must line up with `subs` positionally; the pointers live on the row, not the * parsed blob. */ async function attachCurrentSaves( @@ -916,7 +936,7 @@ async function attachCurrentSaves( subs: SubRoom[], rows: SubRoomRow[] ): Promise { - const saveIds = [...new Set(rows.map((r) => r.current_save_id).filter((id) => id != null))] + const saveIds = [...new Set(rows.map(liveSaveId).filter((id) => id != null))] const byId = new Map() if (saveIds.length > 0) { const placeholders = saveIds.map((_, i) => `?${i + 1}`).join(',') @@ -930,8 +950,10 @@ async function attachCurrentSaves( for (const r of results) byId.set(r.sub_room_data_save_id, parseSubRoomSaveRow(r)) } subs.forEach((sub, i) => { - const id = rows[i]!.current_save_id - sub.CurrentSave = id == null ? null : (byId.get(id) ?? null) + const id = liveSaveId(rows[i]!) + const save = id == null ? undefined : byId.get(id) + if (save) sub.CurrentSave = save + else delete sub.CurrentSave }) } @@ -1302,9 +1324,12 @@ export async function insertSubRoom( ...sub, SubRoomId: subRoomId, RoomId: roomId, - CurrentSave: null, StagedSubRoomDataSaveId: null, } + // A fresh subroom has no save, so the key is absent rather than null — same rule the + // read path applies (see {@link attachCurrentSaves}). The delete also drops the SOURCE's + // save that the spread carried on a clone; the copy below re-points it at its own row. + delete created.CurrentSave // The permission overrides follow the copy too — they live in their own table (keyed by // the id the caller is cloning FROM), so unlike the rest of the settings they aren't // carried by the blob. A fresh subroom (`createSubRoom`) passes no id and copies nothing.