Skip to content
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
4 changes: 2 additions & 2 deletions app/sitemap.test.ts
Original file line number Diff line number Diff line change
Expand Up @@ -214,12 +214,12 @@ describe("the sitemap on an indexable deployment", () => {
}
});

it("keeps a complete list for days (AC-8)", async () => {
it("keeps a complete list for hours, inside the 24 hour IGDB cap (AC-8, spec 0032 AC-11)", async () => {
happyReads();

await sitemap();

expect(cacheLife.mock.calls).toEqual([["days"]]);
expect(cacheLife.mock.calls).toEqual([["hours"]]);
});

it("drops only the pages whose read failed, and keeps it for minutes (AC-8)", async () => {
Expand Down
9 changes: 5 additions & 4 deletions app/sitemap.ts
Original file line number Diff line number Diff line change
Expand Up @@ -54,9 +54,10 @@ const LIST_PATHS = [
* list instead of failing the file (spec 0032, AC-7). `getPopularGames`
* rebuilds its error outside its own cached scope, so `instanceof` holds
* here. Any other rejection, a validation error included, is a bug and is
* rethrown. A complete list lives for days; a partial one for minutes, so the
* full list returns soon after TMDB or IGDB recovers (`lib/seo/AGENTS.md`, the
* sitemap cache rule). Only plain id arrays leave this scope (`AGENTS.md`, the
* rethrown. A complete list lives for hours, not days, because it holds IGDB
* ids and Twitch's terms cap a cached copy at 24 hours (spec 0032, AC-11); a
* partial one lives for minutes, so the full list returns soon after TMDB or
* IGDB recovers (`lib/seo/AGENTS.md`, the sitemap cache rule). Only plain id arrays leave this scope (`AGENTS.md`, the
* cached scope rule).
*/
async function popularTitleIds(): Promise<{
Expand Down Expand Up @@ -100,7 +101,7 @@ async function popularTitleIds(): Promise<{
if (failed) {
cacheLife("minutes");
} else {
cacheLife("days");
cacheLife("hours");
}

return {
Expand Down
36 changes: 36 additions & 0 deletions docs/reviews/2026-10-27-feat-games-launch.md
Original file line number Diff line number Diff line change
@@ -0,0 +1,36 @@
# Review, feat/games-launch (PR #61, merged), 2026-10-27

**Reviewed by**: claude-sonnet-5-5 (author model not recorded)
**Scope**: 66 files, branch vs main (merge base 82102a1, head f2827c0) plus two uncommitted doc ticks (docs/scope/games.md, docs/specs/0032-games-launch/verify.md)
**Verdict**: Approve with nits

## Summary
The change opens games to search engines (canonical, share card and `VideoGame` JSON-LD on the game page, `/games` and the five list pages, game ids in the sitemap with failure isolation), adds the hash-checked IGDB logo and the `IGDB_ATTRIBUTION` credit to the footer and `/terms`, shortens the genre and platform caches to the 24 hour limit, and removes the `NEXT_PUBLIC_GAMES_LIBRARY` flag everywhere. It follows spec 0032 closely and respects the repo's rules: metadata and sitemap read no request state, the not found and failed branches stay `noindex`, the credential and legal-fact boundaries are enforced by tests, and `lib/seo/` stays free of IGDB imports. No blockers or majors. One real inconsistency with the 24 hour caching rule the PR itself adopts (the sitemap's game ids), plus small documentation drift.

## Minor
### 🟡 Sitemap keeps IGDB game ids for days, past the 24 hour cap the PR applies to genres, `app/sitemap.ts:101`
**Problem**: `popularTitleIds` is one cached scope that ends with `cacheLife("days")` when nothing failed. It now holds `gameIds` copied from IGDB's Popular list. The same PR moved the genre and platform reads from `max` to `hours` because the Twitch Developer Services Agreement caps a cached copy of IGDB data at 24 hours (AC-11), and the inner `getPopularGames` read is `hours`.
**Why it matters**: The outer scope outlives the inner one, so a derived copy of IGDB data (ids, and which games are visible/popular) can live well past 24 hours. The data is small and low sensitivity, so the practical risk is low, but the rule is applied unevenly and a hidden game could stay listed for days.
**Suggested fix**: Either record in the rationale why a list of bare ids is not "cached data" under the clause, or cap the outer scope at a lifetime of 24 hours or less when game ids are present (for example `hours` when the list is complete). Note a prerendered sitemap is also frozen until the next deploy, so say so in `lib/seo/AGENTS.md`.

### 🟡 Share-image doc comments still say "TMDB image", `lib/seo/metadata.ts:22`, `:61`
**Problem**: `catalogMetadata`'s `image` is now also an IGDB art URL (`app/games/[id]/page.tsx` passes `artUrl ?? screenshotUrls[0] ?? coverUrl`), but the param docs and the "no TMDB artwork" wording say TMDB only.
**Why it matters**: A future reader may add a host allowlist or validation based on the stale comment, or conclude a non-TMDB URL is a bug.
**Suggested fix**: Reword the docs to "an absolute catalog image URL (TMDB or IGDB)".

## Nits
- ⚪ `lib/seo/metadata.ts:114`, the "failed TMDB read" wording in the list-page doc is TMDB-only; games lists now follow the same rule through `IgdbError`.
- ⚪ `components/layout/site-footer.tsx:78-87`, the logo has `alt="IGDB"` and the link also shows the credit text, so a screen reader hears "IGDB Game data and images from IGDB.com". The spec asks for this (AC-10), so it is only worth a look if you want `alt=""` for the decorative logo inside a link that already has visible text.
- ⚪ `lib/search/params.ts` (`parseSearchParams`), now returns a wide `ParseResult` and is wrapped by two narrowing helpers; its JSDoc could say that callers should prefer the narrowed helpers.
- ⚪ `components/search/navbar-search.tsx:19-20`, the comment now wraps awkwardly after the flag text was removed ("games under `/games`, and the" on its own line).
- ⚪ Working tree: `docs/scope/games.md` still leaves "Review it" unticked, which is correct until this review is accepted; tick it afterwards.

## Strengths
- The failure model of the sitemap is right: `Promise.allSettled`, only `IgdbError` drops the game ids, anything else (including `IgdbValidationError`) is rethrown, and `cacheLife` shortens to `minutes` on a partial result. Errors are rebuilt outside the cached scope so `instanceof` holds.
- Indexing is conservative: only a found, visible game gets a canonical and JSON-LD; not found, hidden and failed branches stay `noindex`, and the canonical is built from the id so a query string never reaches it.
- `catalogListMetadata` handles games' unknown last page explicitly (`hasResults` plus nullable `lastPage`) without changing the TMDB rule.
- Guard rails are encoded as tests: the logo's SHA-256, the one-home rule for `IGDB_ATTRIBUTION`, and a repo-wide test that fails if the removed flag name reappears outside `docs/`.
- Flag removal is complete and simplifies types (`quickSearchRequestSchema` is a plain constant, `TabHrefs` is `Record<LibraryType, string>`, no more optional GAMES tab branch).

## Test coverage
Configured (vitest). The new logic is covered: `gameJsonLd` (including omission of empty fields) in `lib/seo/json-ld.test.ts`; sitemap order, dedupe, 200 cap and IGDB failure isolation in `app/sitemap.test.ts` and `lib/seo/sitemap.test.ts`; game page metadata for found, hidden, failed and invalid ids in `app/games/[id]/page.test.ts`; list metadata results rule in `lib/landing-metadata.test.ts`; footer credit and logo hash; `/terms` content; request-scope tests for the new code. The flag removal updated every affected test and added the leftover-name guard. Not covered: the 24 hour behavior of the sitemap's game ids (no test asserts the outer `cacheLife` for a complete games list), and nothing checks that the share image URL is a real IGDB host, which is fine given the optimizer-independent `og:image`. I did not run the suite; I judged coverage from the diff.
12 changes: 6 additions & 6 deletions docs/scope/games.md
Original file line number Diff line number Diff line change
Expand Up @@ -33,7 +33,7 @@ _This is the second scope for BeStats. The movie and TV scope lives in [scope.md
| G7 | Game search and filters | Slice G4 | done |
| G8 | Game cards everywhere | Slice G4 | done |
| G9 | Richer game page | Slice G5 | done |
| G10 | Games launch | Slice G6 | in-progress |
| G10 | Games launch | Slice G6 | done |

## Foundations

Expand Down Expand Up @@ -178,19 +178,19 @@ spec [0031](../specs/0031-richer-game-page/index.md) · code in `components/game

## Slice G6: Launch

### G10. Games launch · in-progress · GA
### G10. Games launch · done · GA
Turn games on for search engines and production, with the credits IGDB asks for.
**Done when:** game pages and list pages are in the sitemap with `VideoGame` JSON-LD and share cards; an `IGDB_ATTRIBUTION` sits next to `TMDB_ATTRIBUTION` in the footer and on `/terms` with `LEGAL_LAST_UPDATED` bumped; IGDB's current terms are checked again and the result recorded; the `IGDB_*` variables are set in Vercel and the change is deployed through [docs/deploy.md](../deploy.md).
- [x] Design it (spec): `/architect games launch`
- [ ] Build it: `/develop games launch`
- [x] Build it: `/develop games launch`
- [x] Terms check in a browser, recorded in the spec's rationale, plus the logo search, AC-10, AC-11
- [x] Thin thread: an indexable game page (canonical, share card, `VideoGame` JSON-LD) and its ids in the sitemap with failure isolation, AC-1 to AC-3, AC-6, AC-7, AC-14
- [x] `/games` and the five list pages indexed and in the sitemap, AC-4 to AC-6
- [x] IGDB credit in the footer and on `/terms`, `LEGAL_LAST_UPDATED` bumped, AC-8 to AC-10
- [ ] Flag removed, Preview credentials and the deploy docs, then the production launch checks, AC-12, AC-13, AC-15
- [ ] Verify it: `/check verify games launch`
- [x] Flag removed, Preview credentials and the deploy docs, then the production launch checks, AC-12, AC-13, AC-15
- [x] Verify it: `/check verify games launch`
- [x] Test it: `/test games launch`
- [ ] Review it (fresh model): `/check review games launch`
- [x] Review it (fresh model): `/check review games launch`
- [x] Document it: `/document games launch`
spec [0032](../specs/0032-games-launch/index.md) · code in `app/games/`, `app/sitemap.ts`, `lib/seo/`, `lib/legal/`, `components/layout/site-footer.tsx`, `app/terms/`

Expand Down
9 changes: 5 additions & 4 deletions docs/specs/0016-seo-metadata-and-sitemap/index.md
Original file line number Diff line number Diff line change
Expand Up @@ -2,6 +2,7 @@

**Date**: 2026-10-03
**Status**: Accepted
**Amended**: 2026-10-27. Since games joined the sitemap, a complete list lives `hours`, not `days` (AC-8), because it holds IGDB ids; see [spec 0032, AC-7](../0032-games-launch/index.md). A partial list still lives `minutes`. AC-8, the response table, the value sourcing table and build plan step 6 now say `hours` too.

Scope feature: [17. SEO metadata and sitemap](../../scope/scope.md) · Beta tier

Expand Down Expand Up @@ -33,7 +34,7 @@ This finishes how BeStats presents itself to search engines and to link previews

- **AC-6**: On an indexable deployment, `/sitemap.xml` is a valid sitemap of absolute URLs, in this order: `/movies`, `/shows`, then `/movies/{id}` for every movie on `discoverMovies({ page })` pages 1 to 10, then `/shows/{id}` for every show on `discoverTvShows({ page })` pages 1 to 10, each in page order then result order. These are the same calls, with the same default options and popularity order, that the `/movies` and `/shows` landings make, so every listed title is linked from a public page. The sitemap relies on the discover module's existing adult exclusion and does no filtering of its own. Ids are deduplicated within each media type (first occurrence wins), so the file holds at most 402 URLs. An empty page, or a page past TMDB's `totalPages`, simply adds no ids. Entries carry no `lastModified`, `changeFrequency` or `priority`.
- **AC-7**: The sitemap never lists `/search`, a season page, a `?page=` variant, `/showcase`, any private path (`/watchlist`, `/watched`, `/upcoming`, `/account`), any auth path, or any `/api/` or `/auth/` URL.
- **AC-8**: The 20 TMDB page reads run together through `Promise.allSettled` inside one `use cache` function. A read rejected with a `TmdbError` drops only that page's ids; the file still answers 200 with the landings and every id that did load. Any other rejection is rethrown, as `MoviesPage` does, because it is a bug rather than an outage. After all 20 settle, one `if`/`else` calls `cacheLife` exactly once: `"days"` when all 20 succeeded, `"minutes"` when any failed, so a full list returns soon after TMDB recovers. The sitemap's own entry is what lives a day. The inner discover entries keep their own `minutes` lifetime, so the sitemap reuses a landing's TMDB response only when both run within about an hour.
- **AC-8**: The 20 TMDB page reads run together through `Promise.allSettled` inside one `use cache` function. A read rejected with a `TmdbError` drops only that page's ids; the file still answers 200 with the landings and every id that did load. Any other rejection is rethrown, as `MoviesPage` does, because it is a bug rather than an outage. After all 20 settle, one `if`/`else` calls `cacheLife` exactly once: `"hours"` when all 20 succeeded, `"minutes"` when any failed, so a full list returns soon after TMDB recovers. The sitemap's own entry is what lives for hours (it was `"days"` until games joined the sitemap; see the amendment above). The inner discover entries keep their own `minutes` lifetime, so the sitemap reuses a landing's TMDB response only when both run within about an hour.
- **AC-9**: On a deployment that is not indexable, `/sitemap.xml` returns `[]`, which renders as a valid, empty `urlset` (confirmed in the built output), and makes no TMDB request.

*Shared page metadata*
Expand Down Expand Up @@ -110,7 +111,7 @@ Reasoning and options: see [rationale.md](rationale.md).
| Route | Output | Cache | Errors |
|---|---|---|---|
| `/robots.txt` | text, AC-4 or AC-5 | prerendered at build | none |
| `/sitemap.xml` | XML `urlset`, AC-6 to AC-9 | `days`, or `minutes` after any failed read | never non 200; failures shrink the list |
| `/sitemap.xml` | XML `urlset`, AC-6 to AC-9 | `hours`, or `minutes` after any failed read | never non 200; failures shrink the list |
| `/opengraph-image` | 1200 by 630 PNG | prerendered at build | none |

**Value sourcing**:
Expand All @@ -120,7 +121,7 @@ Reasoning and options: see [rationale.md](rationale.md).
| any URL field | the site origin | `NEXT_PUBLIC_SITE_URL` through `siteUrl()` |
| robots, sitemap | indexable or not | `VERCEL_ENV` (set by Vercel per deployment) plus `siteUrl()` |
| sitemap | movie ids, show ids | `discoverMovies({ page })`, `discoverTvShows({ page })` for pages 1 to 10, default options, the landings' calls |
| sitemap | cache lifetime | derived: all 20 reads ok → `days`, else `minutes` |
| sitemap | cache lifetime | derived: all 20 reads ok → `hours`, else `minutes` |
| movie metadata | title, description | unchanged, `loadMovie` (spec 0006, AC-12) |
| movie metadata | share image | `movie.backdropUrl` → `movie.posterUrl` → site card |
| show metadata | share image | `show.backdropUrl` → `show.posterUrl` → site card |
Expand Down Expand Up @@ -163,7 +164,7 @@ Tracer Bullet: the first slice proves the pipe from environment to crawler outpu
3. Add `lib/seo/metadata.ts` (`catalogMetadata()`), `app/opengraph-image.tsx` reading the three tokens from `globals.css`, and the root layout's `metadataBase`, default `openGraph` and `twitter`, satisfies **AC-3**, **AC-10**, **AC-11**, **AC-19**, **AC-20**
4. Wire the movie page: `catalogMetadata()` in the found branch, `noindex` in the failed branch, and `lib/seo/json-ld.ts` (`movieJsonLd()`, `jsonLdScript()`) rendered in the found body, with unit tests for omission and escaping, satisfies **AC-12**, **AC-15**, **AC-16**, **AC-21**, **AC-23**
5. Wire the show and season pages the same way, with `showJsonLd()` on the show page only, satisfies **AC-13**, **AC-14**, **AC-15**, **AC-22**
6. Thicken the sitemap: the 20 discover reads with independent failure, dedupe, ordering and the `days` or `minutes` lifetime inside its `use cache` function, with unit tests on a mocked `lib/tmdb`, satisfies **AC-6**, **AC-7**, **AC-8**
6. Thicken the sitemap: the 20 discover reads with independent failure, dedupe, ordering and the `hours` or `minutes` lifetime inside its `use cache` function, with unit tests on a mocked `lib/tmdb`, satisfies **AC-6**, **AC-7**, **AC-8**
7. Convert `/movies` and `/shows` to `generateMetadata` with descriptions and per page canonicals, and add the `/search` description and site card, satisfies **AC-17**, **AC-18**
8. Proof: `pnpm typecheck`, `pnpm lint`, `pnpm test`, `pnpm build` (route table shows the prerendered shells and the three metadata routes), then in the running app check the heads of every public page type, the not found and failed branches, a private page and an auth page, signed out and signed in, and with `NEXT_PUBLIC_SITE_URL` unset, satisfies **AC-11**, **AC-24**, **AC-25**

Expand Down
Loading
Loading