feat(viewer+engine): browsable full class spell list for prepared casters (#754) - #887
Conversation
…ters (#754) Optimizer persona (2026-06-15 confirm sweep, MAJOR): a Paladin/prepared caster genuinely couldn't PLAN — the Spellbook + Rest & Prepare only showed the FEW currently-prepared/known spells, never the full class spell list to prepare FROM. Engine (additive, derived-only — round-trips; engine stays sole writer): - spells.class_spell_list(class_name, max_level): re-derives a class's full SRD spell list from the srd524 'classes' field (srd-2024_<class> slugs), sorted by (level, name), optionally capped to a slot level. SRD-correct (Paladin L1 == the 13 srd524 paladin L1 spells; half-casters carry no cantrips). - get_character returns preparable_spells: the full preparable pool for the caster's class(es), capped to the highest available slot level (L10 Paladin -> 30 spells across L1-3), multiclass-merged. [] for a non-caster. Never persisted. Viewer (read-only move-sink — no new write path): - /character-surface party shape carries hero.preparableSpells (enriched spell cards), mirroring the engine derivation via the lazily-imported spells module. - SpellbookBrowser shows an 'Available to Prepare' section: the WHOLE class list, each tagged Prepared vs Available, alongside the prepared/known groups. - RestPrepareModal prep step iterates the FULL preparable pool (grouped by level), so the caster can SELECT a spell they haven't prepared; the chosen set rides the existing 'do' /move which now explicitly names prepare_spells (replace) — the engine prepares. Tests: engine test_spellcasting (+5: class_spell_list pool/cap/unknown, get_character preparable_spells for caster + empty for non-caster); viewer test_readmodel_surfaces (+2: browsable pool surfaced + empty for non-caster); new JSX-harness test_spellbook_preparable (+8: prep-step pool browse/select/relay/preselect, spellbook available section + prepared-vs-available marking, non-caster guard). fast_gate: PASS (215). Additive; SRD-correct; round-trip safe.
📝 WalkthroughWalkthroughAdds a "preparable spell pool" feature end-to-end. The engine gains a cached SRD class spell index and a ChangesPreparable Spell Pool Feature
Sequence Diagram(s)sequenceDiagram
participant RestPrepareModal
participant hero.preparableSpells
participant EngineServer
participant spells.class_spell_list
RestPrepareModal->>hero.preparableSpells: read grouped prepPool by level
RestPrepareModal->>RestPrepareModal: seed prepared state from hero.spells["Prepared"]
RestPrepareModal->>EngineServer: relay /move "prepare_spells, replace [selected names]"
EngineServer->>spells.class_spell_list: class_spell_list(class, max_level=highest_slot)
spells.class_spell_list-->>EngineServer: [{name, level}] sorted SRD pool
EngineServer-->>RestPrepareModal: updated character sheet with preparable_spells
sequenceDiagram
participant SpellsTab
participant SpellbookBrowser
participant hero.preparableSpells
participant preparedNames
SpellsTab->>SpellbookBrowser: open with preparable=hero.preparableSpells
SpellbookBrowser->>hero.preparableSpells: build poolByLevel
SpellbookBrowser->>preparedNames: extract from hero.spells["Prepared"]
SpellbookBrowser-->>SpellsTab: render "Available to Prepare" section<br>with "Prepared"/"Available" tags per spell
Estimated code review effort🎯 4 (Complex) | ⏱️ ~60 minutes Possibly related PRs
Suggested labels
🚥 Pre-merge checks | ✅ 3 | ❌ 2❌ Failed checks (2 warnings)
✅ Passed checks (3 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches📝 Generate docstrings
Comment |
![chatgpt-codex-connector[bot]](https://avatars.githubusercontent.com/in/1144995?s=60&v=4){kind=small}
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 0569b91ea8
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| # Only real caster classes contribute a preparable pool (mirror _casting_ability). | ||
| if not cname or cname.lower() not in _CASTING_ABILITY: | ||
| continue | ||
| try: | ||
| entries = mod.class_spell_list(cname, max_level=max_lvl) or [] |
There was a problem hiding this comment.
Because this calls class_spell_list for every caster class, /character-surface now offers the full Wizard/Bard/Sorcerer/Warlock lists whenever they have slots, even when those spells are not in spells_known. The Rest & Prepare modal relays any selected name to prepare_spells, and cast_spell treats a non-empty prepared list as sufficient, so a L5 Wizard with only starter known spells can prepare/cast Fireball without learning it; restrict this pool to classes that prepare from the full class list, or to the character's known spellbook for limited-known casters.
Useful? React with 👍 / 👎.
| } else { | ||
| setPrepared({ ...prepared, [lv]: [...cur, name] }); |
There was a problem hiding this comment.
With the guard changed to an unconditional append, a player can click every entry in the new full pool and the Seal action sends that entire set to prepare_spells; that engine tool only canonicalizes and stores the list, so a L10 Paladin can prepare all 30 visible L1–L3 Paladin spells instead of a daily preparation limit. Please enforce a preparation cap before appending or before relaying the replacement list.
Useful? React with 👍 / 👎.
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 4
🧹 Nitpick comments (1)
viewer/tests/test_readmodel_surfaces.py (1)
462-462: 💤 Low valueConsider defensive assertion before indexing
wyll["spells"][0].The test assumes
wyll["spells"]is non-empty when accessing[0]["list"]. While the character setup includesspells_prepared, adding an explicit assertion would make the test more robust and self-documenting:self.assertTrue(len(wyll["spells"]) > 0, "prepared caster should have at least one spell group") self.assertGreater(len(pool), len(wyll["spells"][0]["list"]))🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@viewer/tests/test_readmodel_surfaces.py` at line 462, Add a defensive assertion before the existing assertGreater call that validates wyll["spells"] contains at least one element. Insert a assertTrue assertion that checks len(wyll["spells"]) is greater than 0, with a descriptive message like "prepared caster should have at least one spell group", immediately before the line containing assertGreater(len(pool), len(wyll["spells"][0]["list"])). This makes the test more robust by explicitly documenting the assumption about the spell data structure and prevents obscure IndexError failures if the assumption is violated.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@viewer/openworlds/screen-character.jsx`:
- Around line 685-701: The React.useEffect hook pre-seeds state from hero.spells
and hero.preparableSpells but only declares hero.id as a dependency, causing the
effect to skip re-running if spells change while the hero ID remains the same.
Add hero.spells and hero.preparableSpells to the dependency array inside the
useEffect call to ensure re-seeding occurs when prepared spells are updated, or
if intentionally preserving in-progress edits is preferred, replace the
eslint-disable-next-line comment with a detailed code comment explaining why
these dependencies are intentionally omitted.
In `@viewer/server.py`:
- Around line 4138-4140: The current filter checks if the class name is in
_CASTING_ABILITY to control access to preparableSpells, but _CASTING_ABILITY
includes all caster classes including non-prepared-casters like Bard, Sorcerer,
and Warlock. Replace the _CASTING_ABILITY reference with a more restrictive
collection that only includes prepared-caster classes (such as Cleric, Wizard,
Druid) that actually support the "prepare from class list" mechanic. This
ensures preparableSpells is only made available for classes that can
legitimately prepare spells.
In `@viewer/tests/test_readmodel_surfaces.py`:
- Line 442: Replace the EN DASH character (–) with a standard ASCII HYPHEN-MINUS
(-) in the comment on line 442. Change "L1–3" to "L1-3" to avoid encoding issues
while maintaining the same meaning in the slot level reference comment.
- Around line 465-466: The test validates that Paladin spell pools cap at level
3 (the maximum), but it does not verify that cantrips (level-0 spells) are
excluded as required. Add an assertion after the existing max-level check to
confirm that the minimum spell level in the pool is at least 1, ensuring no
level-0 spells are present. Reference the same pool variable and sp["level"]
structure used in the existing max assertion.
---
Nitpick comments:
In `@viewer/tests/test_readmodel_surfaces.py`:
- Line 462: Add a defensive assertion before the existing assertGreater call
that validates wyll["spells"] contains at least one element. Insert a assertTrue
assertion that checks len(wyll["spells"]) is greater than 0, with a descriptive
message like "prepared caster should have at least one spell group", immediately
before the line containing assertGreater(len(pool),
len(wyll["spells"][0]["list"])). This makes the test more robust by explicitly
documenting the assumption about the spell data structure and prevents obscure
IndexError failures if the assumption is violated.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro Plus
Run ID: 5c9e6930-4957-4201-940f-d041e3a6a923
📒 Files selected for processing (7)
servers/engine/server.pyservers/engine/spells.pyservers/engine/tests/test_spellcasting.pyviewer/openworlds/screen-character.jsxviewer/server.pyviewer/tests/test_readmodel_surfaces.pyviewer/tests/test_spellbook_preparable.py
| // Pre-seed the picker with the caster's CURRENTLY prepared spells (from hero.spells' | ||
| // "Prepared" group) so opening the modal shows their real preparation, and "Seal" edits it. | ||
| React.useEffect(() => { | ||
| const seed = {}; | ||
| for (const grp of (Array.isArray(hero.spells) ? hero.spells : [])) { | ||
| if (String(grp.level).toLowerCase() !== "prepared") continue; | ||
| for (const sp of (grp.list || [])) { | ||
| // place each prepared spell into its level bucket using the pool's known level | ||
| const inPool = (Array.isArray(hero.preparableSpells) ? hero.preparableSpells : []) | ||
| .find((p) => p.name === sp.name); | ||
| const lv = inPool ? (Number(inPool.level) || 0) : 0; | ||
| (seed[lv] = seed[lv] || []).push(sp.name); | ||
| } | ||
| } | ||
| if (Object.keys(seed).length) setPrepared(seed); | ||
| // eslint-disable-next-line react-hooks/exhaustive-deps | ||
| }, [hero.id]); |
There was a problem hiding this comment.
The useEffect dependency array excludes hero.spells and hero.preparableSpells, risking stale pre-seeding.
The effect pre-seeds prepared state from hero.spells but only depends on hero.id. If the hero's prepared spells change (e.g., after a successful prepare action and surface refresh) while keeping the same hero.id, the picker won't re-seed with the updated preparation.
This may cause the picker to show outdated selections after the surface refreshes. Consider adding hero.spells to the dependency array, or document why re-seeding on spell changes is intentionally avoided (e.g., to preserve in-progress edits).
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@viewer/openworlds/screen-character.jsx` around lines 685 - 701, The
React.useEffect hook pre-seeds state from hero.spells and hero.preparableSpells
but only declares hero.id as a dependency, causing the effect to skip re-running
if spells change while the hero ID remains the same. Add hero.spells and
hero.preparableSpells to the dependency array inside the useEffect call to
ensure re-seeding occurs when prepared spells are updated, or if intentionally
preserving in-progress edits is preferred, replace the eslint-disable-next-line
comment with a detailed code comment explaining why these dependencies are
intentionally omitted.
| # Only real caster classes contribute a preparable pool (mirror _casting_ability). | ||
| if not cname or cname.lower() not in _CASTING_ABILITY: | ||
| continue |
There was a problem hiding this comment.
Restrict preparableSpells to prepared-caster classes only.
On Line 4139, filtering by _CASTING_ABILITY includes classes like Bard/Sorcerer/Warlock, which are casters but not prepared-casters. That makes this pool available where “prepare from class list” is not valid.
💡 Suggested fix
_CASTING_ABILITY = {
"bard": "charisma", "cleric": "wisdom", "druid": "wisdom", "paladin": "charisma",
"ranger": "wisdom", "sorcerer": "charisma", "warlock": "charisma", "wizard": "intelligence",
}
+
+# Classes that prepare spells from a daily class list.
+_PREPARABLE_CASTER_CLASSES = {"cleric", "druid", "paladin", "ranger", "wizard"}
@@
- if not cname or cname.lower() not in _CASTING_ABILITY:
+ if not cname or cname.lower() not in _PREPARABLE_CASTER_CLASSES:
continue🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@viewer/server.py` around lines 4138 - 4140, The current filter checks if the
class name is in _CASTING_ABILITY to control access to preparableSpells, but
_CASTING_ABILITY includes all caster classes including non-prepared-casters like
Bard, Sorcerer, and Warlock. Replace the _CASTING_ABILITY reference with a more
restrictive collection that only includes prepared-caster classes (such as
Cleric, Wizard, Druid) that actually support the "prepare from class list"
mechanic. This ensures preparableSpells is only made available for classes that
can legitimately prepare spells.
| # #754 (optimizer): the Spellbook must let a prepared caster BROWSE the full class spell | ||
| # list (what they can prepare FROM), not just the few currently prepared. The surface | ||
| # projects `preparableSpells` — the whole Paladin list, capped to the caster's highest | ||
| # slot level (L10 Paladin -> L1–3), enriched with the same SRD rules cards. |
There was a problem hiding this comment.
Replace EN DASH with HYPHEN-MINUS in comment.
The comment uses an EN DASH (–) in "L1–3" which can cause encoding issues. Use the standard ASCII hyphen-minus instead.
📝 Proposed fix
- # projects `preparableSpells` — the whole Paladin list, capped to the caster's highest
+ # projects `preparableSpells` - the whole Paladin list, capped to the caster's highest🧰 Tools
🪛 Ruff (0.15.15)
[warning] 442-442: Comment contains ambiguous – (EN DASH). Did you mean - (HYPHEN-MINUS)?
(RUF003)
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@viewer/tests/test_readmodel_surfaces.py` at line 442, Replace the EN DASH
character (–) with a standard ASCII HYPHEN-MINUS (-) in the comment on line 442.
Change "L1–3" to "L1-3" to avoid encoding issues while maintaining the same
meaning in the slot level reference comment.
Source: Linters/SAST tools
| # capped to the highest slot level (L3) — no L4/L5 spells the L10 Paladin can't slot | ||
| self.assertEqual(max(sp["level"] for sp in pool), 3) |
There was a problem hiding this comment.
🛠️ Refactor suggestion | 🟠 Major | ⚡ Quick win
Verify that the Paladin pool excludes cantrips.
Per the PR objectives, "half-casters carry no cantrips." The test verifies the max level but doesn't confirm the absence of level-0 spells. Add an assertion to validate this SRD-correct behavior:
# capped to the highest slot level (L3) — no L4/L5 spells the L10 Paladin can't slot
self.assertEqual(max(sp["level"] for sp in pool), 3)
+# half-casters carry no cantrips (level 0)
+self.assertTrue(all(sp["level"] >= 1 for sp in pool), "Paladin pool must exclude cantrips")📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| # capped to the highest slot level (L3) — no L4/L5 spells the L10 Paladin can't slot | |
| self.assertEqual(max(sp["level"] for sp in pool), 3) | |
| # capped to the highest slot level (L3) — no L4/L5 spells the L10 Paladin can't slot | |
| self.assertEqual(max(sp["level"] for sp in pool), 3) | |
| # half-casters carry no cantrips (level 0) | |
| self.assertTrue(all(sp["level"] >= 1 for sp in pool), "Paladin pool must exclude cantrips") |
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@viewer/tests/test_readmodel_surfaces.py` around lines 465 - 466, The test
validates that Paladin spell pools cap at level 3 (the maximum), but it does not
verify that cantrips (level-0 spells) are excluded as required. Add an assertion
after the existing max-level check to confirm that the minimum spell level in
the pool is at least 1, ensuring no level-0 spells are present. Reference the
same pool variable and sp["level"] structure used in the existing max assertion.
1 participant
Persona complaint (2026-06-15 confirm sweep, optimizer — MAJOR)
A Paladin / prepared caster genuinely could not plan:
prepare_spellstool existed but no viewer step let the caster choose which spells to prepare from a browsable pool.Fix
Engine (
servers/engine/spells.py,servers/engine/server.py) — additive, derived-only, round-trips; engine stays sole writer:spells.class_spell_list(class_name, max_level=None)re-derives a class's full SRD spell list straight from the srd524classesfield (srd-2024_<class>slugs), sorted by(level, name), optionally capped to a slot level. SRD-correct: Paladin L1 == the 13 srd524 paladin L1 spells; half-casters (Paladin/Ranger) carry no cantrips.get_characterreturns a newpreparable_spellsfield — the full preparable pool for the caster's class(es), capped to the highest available slot level (a L10 Paladin → 30 spells across L1–3), multiclass-merged.[]for a non-caster. Never persisted (purely derived).Viewer (
viewer/server.py,viewer/openworlds/screen-character.jsx) — read-only move-sink, no new write path:/character-surfaceparty shape carrieshero.preparableSpells(enriched spell cards), mirroring the engine derivation via the lazily-imported enginespellsmodule.do/movewhich now explicitly namesprepare_spells(replace) — the engine prepares (viewer stays a move-sink, exactly the [camp] Wire RestModal to engine short_rest + prepare_spells #610/[tools] prepare_spells move-kind + relay (E3) #617/fix(viewer): wire Rest & Prepare relay + browsable level-up subclasses (#610 #617 #607) #873 relay pattern). Currently-prepared spells are pre-seeded so "Seal" edits, never silently wipes.Invariants honored
preparable_spellsis a read-only derived field onget_character; never written to the snapshot (verified: not persisted, round-trips stable). The viewer relays adomove — it never writes campaign state.[]for non-casters / old snapshots; nothing else on the sheet changes.data/srd/srd524/Spell.json(the class↔spell map), so the list is never hand-maintained and can't drift./movekind, no env/id changes; reuses the existingdorelay.Tests
servers/engine/tests/test_spellcasting.py(+5):class_spell_listfull pool / max-level cap / unknown-class-empty;get_characterpreparable_spellsfor a L10 Paladin + empty for a Fighter.viewer/tests/test_readmodel_surfaces.py(+2):/character-surfacesurfaces the browsable pool for a prepared caster (enriched, capped to L3) + empty for a non-caster.viewer/tests/test_spellbook_preparable.py(new, +8): JSX-harness over the shippedscreen-character.jsx— prep step lists the full pool / selects a non-prepared spell / relaysprepare_spells/ pre-selects current preparation; SpellbookBrowser shows the Available-to-Prepare section + Prepared-vs-Available marking; non-caster guard.qa/fast_gate.sh: PASS (215 deterministic engine tests). Combined engine+viewer regression: 307 passed.DO NOT MERGE — for review.
Summary by CodeRabbit