Wave 0: depth cheap-wins + Bard skills crit fix (BG3/PFK parity #591) - #625
Merged
Conversation
added 16 commits
June 3, 2026 00:32
…acter sheet (depth cheap-win)
The read-model already computes hero.spellcasting {abilityShort, spellSaveDc, spellAttackBonus}
(viewer/server.py _character_spellcasting, guarded by test_charsheet_depth.py) but
screen-character.jsx had ZERO consumers — the viewer silently dropped the #1 number a caster/
optimizer needs to plan. Render it as a header chip block in SpellsTab (omitted for non-casters).
From the 5-persona depth audit (wf_3af90afc): the optimizer's top-cited data-depth major.
data_status=ui-bug-no-data, effort=S. Viewer static + charsheet tests pass (61).
…sheet (depth cheap-wins) Read-model (_character_sheet) now emits stats.hitDice/hitDiceRemaining (models.Character.hit_dice*) + stats.passivePerception (10 + perception skill bonus) — both derivable from the character model, never previously emitted. Defense block renders them as StatLines (omitted when absent). Adds a regression guard (test_charsheet_depth) asserting the read-model emits them — guards the 'engine has it, viewer drops it' class. Viewer static + charsheet tests pass (62).
…bmit robustness) The disabled check reads busyAction (async state), so two rapid clicks before re-render both pass and double-submit — the adversarial persona's 'Attack target dies on double-click' major. Add a synchronous busyRef gate (set before fetch, cleared in finally). From the depth audit UI-robustness class. effort=S.
…mit robustness) pendingActive (the action-bar gate) only arms after armPending() fires POST-fetch, so two rapid clicks/Enters before that both pass the guard and double-submit /move (engine queue has no dedup). Add a synchronous submittingRef (set before fetch, cleared in finally). The broadest UI-robustness fix — covers every table action across all personas. From the depth audit. effort=S.
…double-submit robustness) pick() and sendFreeForm() fire /move fire-and-forget with no in-flight guard, so a rapid double-click on a parley slot or Send queues duplicate social-check/say intents. Add a synchronous submittingRef (set before fetch, cleared in .finally on the promise chain). Completes the Wave-A double-submit locks (table + combat + merchant-pending + parley). From the depth audit. effort=S.
…eap-win + guard) intel_projection tier-3 (slain) carried hp/abilities/saves/actions but DROPPED the damage_resistances/immunities/vulnerabilities/condition_immunities that stat_block already populates — so a player who slew an Adult Red Dragon couldn't learn it's fire-immune (the single most tactically load-bearing fact). Pass them through at t>=3 (strict gating: tier 2 still hides them); render a 'Defenses' pill block in screen-bestiary.jsx (mirrors Known-abilities). Regression guard added (test_bestiary). Verified: Adult Red Dragon t3 -> immunities=['fire']. effort=S.
…double-submit lock Two bugs the v6 5-persona sweep surfaced on the .app experience: - MK-12 (adversarial): the haggle row rendered the struck list price and the discounted price ADJACENTLY (e.g. 24 then 23 -> reads as one number '2423'). Add an explicit '->' separator + whiteSpace:nowrap so '24 -> 23 gp' is unambiguous. - MK-11 (Wave-A robustness, the merchant surface the lock wave missed): the live (canAct) Confirm path fires POST /move fire-and-forget and only clears the cart in the async .then, so a fast second click relays a SECOND purchase before the first resolves. Add a synchronous submittingRef lock, released in .finally / after the local-preview apply. Viewer static: 54 passed.
…vs-Stash contradiction)
Optimizer's concrete complaint (stable at 5 across all 3 sweeps): 'the coin total
contradicts between Market and Stash.' Root cause: the merchant hardcoded
useState({gp:232,sp:68,cp:14}) while the Stash reads live currency off
/inventory-surface. The merchant already fetches /character-surface — but that
read-model didn't emit currency.
- server.py: add "currency": _currency_for(ch) to the character-surface read-model
(same helper the inventory-surface uses; engine = sole writer).
- screen-merchant.jsx: the displayed + spent-against purse now derives from
surface.currency when live (matches the Stash exactly); the local demo purse is
used ONLY in read-only preview (renamed coins->localCoins for that path).
- guard: test_surface_exposes_currency_for_market_purse asserts the surface emits
live currency (cp/sp/ep/gp/pp) so this can't silently regress.
charsheet-depth 9 passed; viewer static 54 passed.
… kills Optimizer's #1 complaint (stable at 5 across all 3 sweeps): 'the Codex bestiary is fully fog-of-war locked with zero creature names or stats.' Root cause: the intel codex (#263) only reveals creatures the party has SIGHTED/ENGAGED/SLAIN, so in real play (where you rarely slay enough) it reads as empty. The engine ALREADY supports a public browse — player_bestiary(intel=None) returns the global SRD preview (name + CR + preview stat line) for every creature. This just exposes it: - server.py build_bestiary_response: add reference=False; when set, bypass campaign intel (intel stays None) -> the global SRD browse. Route reads ?reference=1. - screen-bestiary.jsx: a 'Browse all' toggle (off=earned-intel fog-of-war, on=?reference=1 public reference) + widen the page to 50. - guard: test_reference_mode_browses_all_bypassing_intel proves ?reference=1 returns NAMED creatures for a campaign that has slain nothing (vs redacted without it). Validated: reference browse returns 50 named creatures (Aboleth, Adult Black Dragon...); intel={} default is all-redacted. bestiary-surface 7 passed; viewer static 54 passed.
…ties view) Optimizer complaint: 'Market items have no properties or compare pane.' The wares table rendered name/type/weight/price per row but offered no way to inspect an item. Add an 'Item Detail' pane in the LEFT panel that shows the last-hovered ware's facts (image, name, type, weight, price, and a description when present). Bound to a detailItem state set on row mouseEnter — it persists after mouse-leave (hoverItem still drives only the row tint), so the pane doesn't blank when the cursor moves to read it. Honest empty-state until a row is hovered. Completes the merchant cluster (haggle arrow + Confirm-lock + live purse + detail pane). Viewer static: 54 passed.
Feature-research top quick-win (value 7 / effort 1 / ratio 7 — highest in the backlog). Each skill-check slot already shows its modifier + suggested_dc; add a color-coded success-% chip derived purely client-side: faces = clamp(21-(dc-mod), 1, 19), pct = faces/20 (nat-1 always fails, nat-20 always succeeds -> 5..95%). Green >=65, amber >=35, red below. Tooltip spells out 'd20+mod vs DC -> ~N%'. No engine/read-model change. Makes the roll legible the way BG3 does — newbie+optimizer both reach for it. Viewer static: 54 passed.
…as blank)
Feature-research quick-win (value 8 / effort 2). data/srd/class_features.json carries
260 canon feature descriptions but _character_sheet projected classFeatures as
{name, detail:''} — so the Abilities/Feats tabs (which already render c.detail) showed
bare name lists. Add a cached global name->desc map and join it; honest empty when a
feature has no authored desc (subclass features the file lacks stay blank, never faked).
Turns two bare name-lists into BG3-style inspectable feature cards with zero JSX change
and zero fabrication. Updated the guard to assert the detail is now populated.
charsheet-depth 9 passed; desc map = 145 entries.
…sidebar Feature-research quick-win (value 8 / effort 2). The atlas surface ships danger(0-10)/difficulty/route_kind/tags per travel edge (edgeStyle already reads them for the map lines), but AtlasSidebar rendered only 'X minutes away'. Add a route readout under the distance: road-type pill, difficulty (crimson if hard/treacherous/deadly), a color-coded Danger N/10 pill (green<3, amber, crimson>=6 with a warning glyph), and edge tags. The BG3/PFK 'do I dare take this road' read — pure presentation, data already on the wire. Viewer static: 54 passed.
…row->lead loop Feature-research quick-win (value 8 / effort 2). The engine Faction model carries rank/joined/questline_arc_id (models.py:902, set by join_faction/advance_faction_arc) but _relations_factions dropped them, so the Skyrim/PFK 'join a faction, climb its ranks, lead it' loop was invisible — only a reputation bar showed. - server.py: project rank/joined/questlineArcId onto the factions surface (additive). - screen-relations.jsx: a 'Membership: Member - Rank N' StatLine in the faction detail when joined (rank 0 == joined but unranked), gated into the standing block. Viewer static: 54 passed.
… 0 skills — crit)
sweep_v7 optimizer crit (G2): a level-1 Bard rendered 0 skill proficiencies and the
min-maxer bailed at turn 2. Root cause: srd_tables.class_skills('bard') = {count:3,
from:['any']} (5e Bards choose ANY 3 skills), and create_character sliced that pool
literally -> persisted skill_proficiencies=['any'], a placeholder that matches no real
skill, so the sheet showed nothing. Rogue/Fighter/etc. carry concrete skill lists and
were unaffected (which is why only a Bard run surfaced it).
Fix: when the class skill pool contains 'any', expand it to the full SKILL_ABILITIES
list (keeping any explicit real skills first) before taking count. Bard now persists 3
concrete skills. Guard: test_any_skill_class_resolves_to_concrete_skills.
Validated standalone: bard -> ['acrobatics','animal_handling','arcana'] (all concrete);
rogue/fighter/wizard unchanged.
… clean, integrate before squash-merge
|
Warning Review limit reached
More reviews will be available in 1 minute and 39 seconds. Learn how PR review limits work. Your organization has run out of usage credits. Purchase more in the billing tab. ⌛ How to resolve this issue?After more reviews become available, a review can be triggered using the We recommend that you space out your commits to avoid hitting the rate limit. 🚦 How do rate limits work?CodeRabbit enforces hourly rate limits for each developer per organization. Our paid plans include higher PR review limits than trial, open-source, and free plans. In all cases, reviews become available again over time. During sustained high-volume PR review activity, CodeRabbit may temporarily slow when the next review becomes available. Please see our Fair Usage Limits Policy for further information. ℹ️ Review info⚙️ Run configurationConfiguration used: Organization UI Review profile: CHILL Plan: Pro Plus Run ID: 📒 Files selected for processing (1)
📝 WalkthroughWalkthroughEngine now exposes tier-3 creature defensive fields and fixes SRD class skill resolution for "any" skill pools. Viewer adds bestiary reference browse-all mode, enriches character sheets with computed stats and spellcasting info, hardens submission flows against double-clicks across multiple screens, and adds skill probability estimates, route metadata, merchant item detail, and faction membership displays. ChangesEngine: Defensive tiers and skill resolution
Viewer: Bestiary reference mode and defenses
Viewer: Character enrichment
Viewer: Multi-screen submission hardening and interactive UI
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes Possibly related PRs
🚥 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. Comment |
…(summary) CI caught my own guard failing (assert []): the engine 'any'-skill fix is correct (the Bard gets 3 concrete skills), but the test read get_state(cid)['party'] — which is a VITALS SUMMARY (id/name/hp/ac/conditions) that never carried skill_proficiencies. Switch to server.get_character(cid, id) (the documented full sheet = ch.model_dump). Classic wrong-surface test antipattern; the fix under test never changed.
This was referenced Jun 9, 2026
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Wave 0 — depth cheap-wins + the Bard skills crit fix (BG3/PFK parity sprint, #591)
Lands the "done this session" depth work. All additive, all tested, the read-model debt the feature-research flagged (engine computes it, viewer dropped it). Integrated with origin/main (incl. #583) — clean, no conflicts.
Cheap-wins (Lever-1 projection debt):
/character-surface(kills the Market-vs-Stash contradiction), item detail pane, haggle price arrow (no "2423"), Confirm double-submit lockCritical fix:
fix(engine): resolve the['any']skill placeholder at creation — a level-1 Bard rendered 0 skills (class_skills('bard') = {from:['any']}sliced literally) → min-maxer bailed. Now expands to concrete skills. Guard added.Measured (VM 5-persona part-B): AVG satisfaction 6.40 → 6.60 (best trend); the Bard skills crit is gone. Remaining crits (#623 DM-beat reliability, #624 Wizard subclass) are separate pre-existing gaps, not regressions.
8 new regression guards across viewer-tests + engine pytest. Squash-merge.
Summary by CodeRabbit
New Features
Bug Fixes