feat(spells): required ?edition= on GET /api/spells + SpellClass className index

[Sandersland]

Summary

• F3 of epic Epic: 2014 spell catalog — Spell.classes is un-editioned 2024 content, so a 2014 Bard is offered the 2024 Bard list #1517 (foundation slice 3/3), reversing Follow-up: give encumbrance and starting-spell rules a backend implementation (#1272) #1377's "no ?edition=" on GET /api/spells. The route now 400s without ?edition= (absent or unrecognized), applying withEditionOrShared + resolveEditionCatalog (catalog-edition.ts) so a genuine 2014/2024 fork resolves to exactly one row per name — the same shape featsRouter/referenceRouter already use.
• Threads edition through the two character-scoped read paths that already compute editionOf(char): resolveCreationSpells (character-create.ts) and the level-up learn path (loadPickCatalogRows/assertOnSpellList, level-up-transaction.ts). Both gain a new rejectCrossEditionSpellForks admission check (lib/spellcasting/spell-classes.ts) — it rejects a submitted spell id only when the requesting edition actually resolves a different row for that name, not on a bare tag mismatch. This matters because today's real catalog (~109 rows) is uniformly EDITION_2024-tagged with no 2014 counterpart yet; a strict crossEditionRejection-style check would have rejected every 2014 spell pick and regressed feat(leveling): 2014 known-caster level-up spell picks, Ranger swap, known-vs-prepared copy #1729's just-shipped 2014 known-caster level-up. Verified empirically both ways (see Concerns below).
• Updates every frontend/src/api caller chain of the spells endpoint to pass the viewing character's (or creation draft's) edition — no unfiltered browse remains.
• Folds in 2014 spell catalog F2: replace Spell.classes String[] with a SpellClass join (coordinate shape with #1495) #1711's review follow-up: SpellClass gains @@index([className]) via a hand-written migration (Prisma 7.8 gates DDL), ahead of the 2014 content slices (2014 spell catalog CS: transcribe PHB'14 shared / multi-list spells (3+ class lists), authored once #1713-2014 spell catalog: transcribe the PHB'14 Ranger spell list (levels 0-9) #1721) multiplying the join's row count.

Required-param contract

GET /api/spells?edition= — same shape as featsRouter/referenceRouter (#1411/#1412):

• Absent → 400 "Missing required query parameter: edition"
• Unrecognized → 400 "Unknown edition: <value>"
• ?class=/?maxLevel= remain optional, applied inside the edition-scoped query.

Index migration

Hand-written (20260804231500_spell_class_class_name_index/migration.sql) rather than prisma migrate dev-generated, matching #1711's own join migration precedent — Prisma 7.8 gates DDL against this schema. Pure additive CREATE INDEX, applied + verified via prisma migrate status (no drift) in the worktree DB.

Tests (RED → GREEN)

• spells.test.ts: new describe blocks for the required-param 400s and a genuine fixture fork (2014+2024 rows sharing a name) proving disjoint resolution — confirmed RED (5/5 failing) by stashing the route change, then GREEN.
• character-create-spells.test.ts / level-up-transaction.test.ts: new fork-rejection tests (fixture rows only, since no real 2014 spell content exists yet) — confirmed RED by temporarily removing the rejectCrossEditionSpellForks call, then GREEN.
• All existing 2014-edition spell tests in both files (Bard Magical Secrets Bug: learnSpell level-up picks are gated only client-side — no class-list or spell-level check on the server #1440, 2014 level-up spell picks: known-caster deltas, the Ranger swap, and known-vs-prepared copy #1509 known-caster level-up, 2014 level-1 creation spell picks: edition-aware counts and no spells step for a 2014 Paladin/Ranger #1510 creation counts) pass unchanged — proof the new check doesn't regress today's real (unforked) catalog.

Frontend callers updated

fetchSpells/useSpellCatalog now take a required edition. Threaded through: CreationSpellsStep, SpeciesCantripSection (creation draft's rulesEdition), NewSpellsStep, ReviewStep's spell-name resolver (character.rulesEdition), SpellCatalogTab → AddSpellPanel → SpellsSection (character's rulesEdition), and CapabilityEditor (DM item authoring) → threaded up through CampaignItemFields/CampaignItemForm from CampaignItemsPanel's existing campaign edition prop.

Drifted refs

The issue's line numbers (character-create.ts:1304-1333, level-up-transaction.ts:404-439) predate #1711 (SpellClass join) and #1729 (2014 known-caster level-up), both merged into this branch's base. Re-grepped by symbol per the build brief.

Concerns

• The admission check (rejectCrossEditionSpellForks) is deliberately not a strict row.edition === edition gate — see its docstring for the full reasoning. Worth a second look: this is a judgment call favoring today's shipped behavior over a stricter (but currently catalog-incompatible) interpretation of "a 2014 Bard must never see a 2024-only row." Once real 2014 spell content lands (2014 spell catalog CS: transcribe PHB'14 shared / multi-list spells (3+ class lists), authored once #1713-2014 spell catalog: transcribe the PHB'14 Ranger spell list (levels 0-9) #1721), this check starts doing real work; until then it's inert on the real catalog (proven only by the fixture-fork tests).
• resolveSpeciesCantripGrant (character-create.ts, High Elf's Cantrip) shares creationPickError with resolveCreationSpells but was left out of this fork check — same latent gap, out of the issue's explicit two-function scope.

Closes #1712

[claude]

Claude finished @Sandersland's task in 4m 27s —— View job

---

Code Review: feat(spells): required ?edition= on GET /api/spells + SpellClass className index

Tasks

• Gather PR context
• Read key changed files (route, spell-classes, character-create, level-up, frontend, tests, migration)
• Analyze backend changes
• Analyze frontend changes
• Analyze tests
• Analyze migration
• Compile findings

---

Overall Assessment

This is a clean, well-reasoned PR. The required-edition contract is consistent with featsRouter/referenceRouter (#1411/#1412), the rejectCrossEditionSpellForks logic is carefully documented, and the test strategy (RED→GREEN confirmation + fixture forks) is solid. The migration is a pure additive index — no destructive DDL.

Two findings worth calling out; neither is a blocker.

---

Finding 1 — Latent stale-spells in CapabilityEditor on edition change

File: frontend/src/features/entities/CapabilityEditor.tsx — line 25

useEffect(() => {
  if (needSpells && spells.length === 0) {   // ← guard skips re-fetch when spells are loaded
    fetchSpells(edition).then(setSpells).catch(() => setSpells([]));
  }
}, [needSpells, spells.length, edition]);

When edition is added to the deps array (which is the right call — the route now 400s without it), the effect re-fires on edition changes. But the spells.length === 0 inner guard means: once the list is populated for edition A, a prop change to edition B is silently skipped and the A-edition catalog stays on screen.

In practice this is inert — a CampaignItemsPanel's edition prop comes from the campaign record and never changes during the lifetime of the mounted component. But the guard pattern is fragile: useSpellCatalog avoids this entirely by never gating on "already fetched" state (it re-fetches unconditionally when deps change). The safest fix is to reset the state when edition changes:

useEffect(() => {
  if (!needSpells) return;
  setSpells([]);
  fetchSpells(edition).then(setSpells).catch(() => setSpells([]));
}, [needSpells, edition]);

Fix this →

---

Finding 2 — resolveSpeciesCantripGrant gap acknowledged but worth tracking

The PR description explicitly flags this: resolveSpeciesCantripGrant shares the same creationPickError path as resolveCreationSpells but doesn't call rejectCrossEditionSpellForks. The function does a bare findUnique on the submitted id with no cross-edition fork check:

// character-create.ts:1530
const raw = await prisma.spell.findUnique({
  where: { id: speciesCantripId },
  include: SPELL_CLASS_MEMBERSHIP_SELECT,
});

Once real 2014 cantrip rows land (#1713+), a client could submit a 2024-only cantrip id to a 2014 High Elf and have it admitted here while resolveCreationSpells would reject it. The PR author is right that this is out of scope for #1712's two-function scope — but a follow-up issue tracking this would be worth filing before #1713 ships.

---

Everything Else — Looks Good

rejectCrossEditionSpellForks logic: The "reject only when a better candidate exists for the requesting edition" design is sound. resolveEditionRow correctly returns undefined when no preferred alternative exists, so the function never fires on today's unforked catalog. The single batched name-lookup (one extra query, not one per mismatched row) is the right efficiency call.

Route (spells.ts): requireEditionOr400 → withEditionOrShared → resolveEditionCatalog is the exact established pattern. The ?class=/?maxLevel= optional handling is unchanged.

loadPickCatalogRows: Correctly threads edition through and adds edition: true to the select so rejectCrossEditionSpellForks receives the data it needs. The two-query cost (initial row load + name lookup for mismatched subset) is minimal and documented.

resolveCreationSpells: Uses include: SPELL_CLASS_MEMBERSHIP_SELECT (full Spell scalars + relation), which naturally includes edition on the returned rows. Type-correct even though it differs from loadPickCatalogRows's explicit select.

Frontend callers: All caller chains correctly thread the viewing character's rulesEdition (or draft's rulesEdition in creation). The if (!rulesEdition) return null guard in CreationCeremony's SpellsStepBody is the right narrowing for the RulesEdition | null draft type.

ReviewStep.tsx: The memoised spellFetcher pattern correctly follows the [needsSpells, edition] dependency — the same "keyed on a boolean, not on array identity" fix that maneuvers and feats already use.

Migration: Pure additive CREATE INDEX "SpellClass_className_idx" — no destructive DDL, matches the hand-written migration precedent set by #1711. The SQL comment correctly explains why ?class= lookups can't use the existing composite index.

Tests: Fork-resolution tests (both route and admission) correctly prove the mechanism with fixture rows while existing 2014 tests remain GREEN — exactly the distinction the PR description calls out.

---

[claude]

Claude finished @Sandersland's task in 4m 0s —— View job

---

Code Review: feat(spells): required ?edition= on GET /api/spells + SpellClass className index

Tasks

• Gather PR context
• Read key changed files (route, spell-classes, character-create, level-up, frontend, tests, migration)
• Analyze backend changes
• Analyze frontend changes
• Analyze tests and migration
• Compile findings and post review

---

Overall Assessment

A clean, well-reasoned PR. The required-edition contract matches the featsRouter/referenceRouter precedent, the resolveSpellCatalogForEdition graceful-fallback posture is correctly justified and empirically verified, and the rejectCrossEditionSpellForks admission check is carefully documented. The RED→GREEN test discipline (fixture forks + existing 2014 tests staying green) is exactly right. Two standing issues worth addressing — one a leftover from the prior review.

---

Finding 1 — CapabilityEditor.tsx stale-spell guard (flagged in prior review, still present)

File: frontend/src/features/entities/CapabilityEditor.tsx — line 26

useEffect(() => {
  if (needSpells && spells.length === 0) {   // ← guard prevents refetch when edition changes
    fetchSpells(edition).then(setSpells).catch(() => setSpells([]));
  }
}, [needSpells, spells.length, edition]);

The spells.length === 0 inner guard means edition changes are silently ignored once the list is populated. This was called out in the prior review and the fix wasn't applied before this sync. In practice it's inert (a CampaignItemsPanel's edition prop comes from the campaign record and never changes during a mounted component's lifetime), but the pattern diverges from useSpellCatalog's unconditional-refetch on deps-change. The spells.length dep also creates a secondary fragility: if a user removes all castSpell capabilities (setting needSpells = false, which resets nothing in state), then adds one back, the guard re-checks spells.length — but spells state hasn't been cleared, so the guard skips the re-fetch even for needSpells false→true transitions.

Suggested fix — reset state on edition change, unconditional fetch when needSpells:

useEffect(() => {
  if (!needSpells) return;
  setSpells([]);
  fetchSpells(edition).then(setSpells).catch(() => setSpells([]));
}, [needSpells, edition]);

Fix this →

---

Finding 2 — resolveSpeciesCantripGrant fork gap (acknowledged, needs a tracking issue before #1713)

File: backend/src/lib/character/character-create.ts — line 1530

const raw = await prisma.spell.findUnique({
  where: { id: speciesCantripId },
  include: SPELL_CLASS_MEMBERSHIP_SELECT,
});
// No rejectCrossEditionSpellForks call here

The PR description explicitly flags this. resolveSpeciesCantripGrant does a bare findUnique without the fork check that resolveCreationSpells (line 1489) now has. Once a real 2014 cantrip row lands (#1713), a client could submit a 2024-only cantrip id to a 2014 High Elf and have it admitted at this seam while resolveCreationSpells would reject the same id. The PR author is right that this is out of the two-function scope for #1712 — but it should get a tracking issue filed before #1713 ships content, not after, since fixing it post-content is harder to red-green verify.

---

Everything Else — Looks Good

resolveSpellCatalogForEdition: The exact ?? shared ?? group[0] fallback is safe. With @@unique([name, edition]) enforced on Spell, a name group can only have one row per (name, edition) pair, so the group[0] fallback — reached only when neither an exact-edition nor a shared (null-edition) row exists — is a single, unambiguous row. The route's orderBy: [{ level: "asc" }, { name: "asc" }] provides stable input ordering. The fork disjointness test in spells.test.ts proves the exact-match branch wins for genuine 2014/2024 pairs, leaving the group[0] branch to the real-catalog case (one EDITION_2024 row per name, served to both editions until #1713–#1721).

rejectCrossEditionSpellForks: The row.edition! non-null assertion on line 69 is safe — mismatched is filtered at line 51 to row.edition !== null && row.edition !== edition, so edition is guaranteed non-null there. The one-batched-query-per-call rather than per-mismatched-row is the right efficiency call.

Route (spells.ts): requireEditionOr400 → resolveSpellCatalogForEdition is the correct adaptation of the feats/reference pattern for the mid-migration catalog. The docstring correctly explains why resolveEditionCatalog couldn't be used here. The 400 on absent/unrecognized edition is tested with RED→GREEN confirmation.

loadPickCatalogRows: Correctly spreads edition: true into the select so rejectCrossEditionSpellForks receives the data it needs. The ...SPELL_CLASS_MEMBERSHIP_SELECT spread into a Prisma select is the correct pattern.

resolveCreationSpells: Uses include: SPELL_CLASS_MEMBERSHIP_SELECT (full scalars + relation). rejectCrossEditionSpellForks only needs { id, name, edition } from each row, which are present on the full Spell type. Type-correct.

ReviewStep.tsx: spellFetcher is correctly memoised on [needsSpells, edition] — the same "keyed on boolean, not array identity" fix that maneuvers and feats already use, and confirmed by the comment citing the prior #1412 pattern.

useSpellCatalog: Correct destructuring of filter primitives (className, maxLevel) into the effect deps — avoids the object-identity refetch loop.

Migration: Pure additive CREATE INDEX "SpellClass_className_idx" — no destructive DDL, correct precedent from #1711. The SQL comment explains the (spellId, className) composite's leading-column limitation clearly.

Tests: The fork-disjointness suite (fixture rows, not real catalog rows) correctly proves the mechanism without requiring 2014 content to exist. The "lone 2024-tagged row served to both editions" test proves the graceful fallback. The existing 2014 tests (Bard Magical Secrets, #1729 known-caster level-up, #1510 creation counts) staying green is the critical non-regression proof.

---