fix(viewer): strip internal routing tag leaking into the chronicle (#410); verify roster/label reports - #410
Merged
Merged
Conversation
|
Caution Review failedPull request was closed or merged during review No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Organization UI Review profile: CHILL Plan: Pro Plus Run ID: 📒 Files selected for processing (2)
🚧 Files skipped from review as they are similar to previous changes (1)
📝 WalkthroughWalkthroughPlayer dialog text displayed in the chat polling beat now applies ChangesRouting Tag Stripping for Player Dialog
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes Suggested labels
Poem
🚥 Pre-merge checks | ✅ 2 | ❌ 3❌ Failed checks (3 warnings)
✅ Passed checks (2 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches📝 Generate docstrings
Comment |
100yenadmin
pushed a commit
that referenced
this pull request
May 30, 2026
…icle (#410) The chronicle showed the player's own action with its internal write-lane routing tag, e.g. `"[do] Without making it obvious…"` (also [say]/[check]/…). The optimistic echo (postMove) already stripped the tag, but the /chat replay of the player's logged line (app.jsx useLiveSession: it.role === "player" -> dialog row) used `it.text` raw. The engine keeps the tag on the logged line for move classification, so once that line round-tripped through /chat it leaked into the transcript (LogEntry's dialog/action branches render entry.text verbatim — only the narration branch sanitizes). Add a shared `stripRoutingTag(text)` helper (registered on window for the table screen + tests), apply it on the /chat player-replay path, and refactor the postMove echo to call the same helper so both player-line render paths stay in lock-step. Display-only — the write lane keeps the tag for engine routing. Strips the known move verbs (say/do/check/save/continue/attack/cast/use_item/clarify), case-insensitive, leaves untagged text + mid-line brackets untouched, null-safe. Test: viewer/tests/test_player_action_tag_strip.py (pytest; brace-matches the fn out of app.jsx and exercises it under Node, mirroring test_sanitize_narration) — the CI viewer-tests job runs `python -m pytest viewer/tests`. 17 pass locally.
100yenadmin
force-pushed
the
fix/roster-and-ui-leaks
branch
from
May 30, 2026 21:01
28a7e99 to
deb39f4
Compare
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 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/app.jsx`:
- Around line 370-374: The code calls stripRoutingTag(it.text) when rendering
player dialog but no global helper is defined, causing a ReferenceError; add a
window.stripRoutingTag function (next to the existing window.neutralizeMarkup
registration) that removes leading routing tags like "[do] " and returns the
cleaned string so the player dialog rendering path (the logic invoking
stripRoutingTag in the dialog mapping) and the tag-strip tests pass; ensure the
function is exported on window as stripRoutingTag and matches the behavior
asserted in viewer/tests/tag-strip.test.js.
🪄 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: 777e54e6-35c1-4a2a-a1fb-cd0124ad0e71
📒 Files selected for processing (2)
viewer/openworlds/app.jsxviewer/tests/tag-strip.test.js
There was a problem hiding this comment.
♻️ Duplicate comments (1)
viewer/openworlds/app.jsx (1)
370-374:⚠️ Potential issue | 🔴 Critical | ⚡ Quick win
stripRoutingTagis still undefined at call site (runtime break + test contract break).Line 374 invokes
stripRoutingTag(it.text), but there is nofunction stripRoutingTag(definition orwindow.stripRoutingTagregistration in this file. This will throw at runtime on player dialog rendering and fails the new regression tests expecting both declarations.Suggested minimal fix
+function stripRoutingTag(text) { + if (typeof text !== "string") return ""; + const routingVerbs = ["say", "do", "check", "save", "continue", "attack", "cast", "use_item", "clarify"]; + const pattern = new RegExp(`^\\[(${routingVerbs.join("|")})\\]\\s*`, "i"); + return text.replace(pattern, ""); +} +window.stripRoutingTag = stripRoutingTag;🤖 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/app.jsx` around lines 370 - 374, The call to stripRoutingTag(it.text) is failing because stripRoutingTag is not defined; add a small helper function named stripRoutingTag that accepts a string and returns it with any leading routing tag (e.g. a bracketed token like "[do] ") removed (trim safely and return original if no tag), place it in the same module (or import it) so the call site can use it, and if other code expects a global, also register it as window.stripRoutingTag = stripRoutingTag; ensure the function name matches exactly so the runtime and tests pass.
🤖 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.
Duplicate comments:
In `@viewer/openworlds/app.jsx`:
- Around line 370-374: The call to stripRoutingTag(it.text) is failing because
stripRoutingTag is not defined; add a small helper function named
stripRoutingTag that accepts a string and returns it with any leading routing
tag (e.g. a bracketed token like "[do] ") removed (trim safely and return
original if no tag), place it in the same module (or import it) so the call site
can use it, and if other code expects a global, also register it as
window.stripRoutingTag = stripRoutingTag; ensure the function name matches
exactly so the runtime and tests pass.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro Plus
Run ID: 203230f2-9a49-477a-b7bd-78da38a04ae5
📒 Files selected for processing (2)
viewer/openworlds/app.jsxviewer/tests/test_player_action_tag_strip.py
…icle (#410) The chronicle showed the player's own action with its internal write-lane routing tag, e.g. `"[do] Without making it obvious…"` (also [say]/[check]/…). The optimistic echo (postMove) already stripped the tag, but the /chat replay of the player's logged line (app.jsx useLiveSession: it.role === "player" -> dialog row) used `it.text` raw. The engine keeps the tag on the logged line for move classification, so once that line round-tripped through /chat it leaked into the transcript (LogEntry's dialog/action branches render entry.text verbatim — only the narration branch sanitizes). Add a shared `window.stripRoutingTag(text)` helper (window-guarded global, like neutralizeMarkup, so the table screen + tests can reach it), apply it on BOTH player-line render paths (the /chat replay and the postMove optimistic echo). Display-only — the write lane keeps the tag for engine routing. Strips the known move verbs (say/do/check/save/continue/attack/cast/use_item/clarify), case-insensitive, leaves untagged text + mid-line brackets untouched, null-safe. Test: viewer/tests/test_player_action_tag_strip.py (pytest; brace-matches the fn out of app.jsx and exercises it under Node, mirroring test_sanitize_narration) — the CI viewer-tests job runs `python -m pytest viewer/tests`. 28 pass locally; 98 green across the related viewer suites.
100yenadmin
force-pushed
the
fix/roster-and-ui-leaks
branch
from
May 30, 2026 21:05
deb39f4 to
dbcabf4
Compare
100yenadmin
added a commit
that referenced
this pull request
May 31, 2026
…and class features (#optimizer) (#416) Two read-model/display data-depth gaps an optimizer persona flagged as critical in a built-.app playtest. Both are DISPLAY gaps — the engine has the data, the character screen did not surface it. Read-model + viewer only; no engine writes. Bug 1 — Spells tab never showed the character-level Spell Save DC / Spell Attack Bonus. PR #410 resolved per-spell saveDc, but there was no once-at-the-top caster summary. Added _casting_ability/_spell_attack_bonus/_character_spellcasting to viewer/server.py (reusing the #410 DC formula; mirrors engine server.spell_save_dc: DC = 8 + prof + mod, attack = prof + mod), exposed as hero.spellcasting on the character surface, and rendered a SpellcastingHeader at the top of SpellsTab. Non-caster -> None -> header omitted (no fabricated DC 0). Bug 2 — Abilities tab read "No active abilities recorded" for a L3 Wizard. The engine DOES populate class/subclass features as NAMES (Character.features via srd_tables.features_through), already surfaced by the read-model as classFeatures — but AbilitiesTab only read hero.abilities (always []) and class features were only shown on the Feats tab. AbilitiesTab now renders hero.classFeatures + honest class/subclass/level context. Names only — the engine does not model feature DESCRIPTIONS or racial traits, so those stay absent (never fabricated). Tests: viewer/tests/test_charsheet_depth.py (8) — caster DC/attack present, non-caster omitted cleanly, DC/attack track prof+ability, class-feature names surfaced + empty-when-engine-has-none. Full viewer suite: 231 passed. Do NOT close on merge — verify on the next 5-persona sweep (optimizer >=7, 0 critical). Co-authored-by: Eva <arncalso@gmail.com>
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.
Why
The 5-persona built-app sweep capped the two power personas — optimizer 5/10, veteran 6/10 (both completed the session, no give-up, just rated low) — on data depth: the inspector UI didn't show the real data the engine already has. This PR surfaces and displays that real data.
Principle: HONEST data only. Every field is sourced from the engine's bundled SRD data (proficiencies on the character sheet,
servers/engine/itemcatalog.pyfor items,data/srd/srd524/Spell.jsonviaspells.srd_spellfor spells). A catalog/SRD miss degrades to today's behavior (empty/name-only) — no fabricated numbers. Additive throughout (empty/missing data == current behavior).Files touched (only these):
viewer/server.py,viewer/openworlds/screen-character.jsx,viewer/openworlds/screen-inventory.jsx, plus additive tests inviewer/tests/test_readmodel_surfaces.py.The 4 bugs — what was surfaced + displayed
1. Skills tab had no proficiency/expertise markers (optimizer + veteran)
proficient/expertiseper skill — no change needed (the data was already exposed).screen-character.jsxSkillsTab): the existing 7px proficiency dot was being missed by power players. Made the marker unmissable — dot + a text badge (Prof/Expertise) + a gold left-accent bar on trained skills + a legend + a "N proficient · M expertise" count. Untrained skills read plainly.2. No equipment paper-doll (veteran)
Wornlist._equipped_itemshelper carrying each equipped item's real catalog stats (kind / damage / damageType / AC / rarity / attunement).screen-character.jsx): replaced the flat list with a slotted paper-doll (HeroEquipDoll) that reuses the inventory screen's canonical slot set + assignment (window.EQUIP_SLOTS/window.assignEquipSlots— no second mechanism), with each cell showing the item's real stat (e.g.1d8 piercing,AC 18) in caption + tooltip.3. Spell inspector was a flat name-list — no rules text (optimizer)
spells.srd_spell: level, school, range, casting time, duration, concentration, ritual, save ability, caster's computed save DC (8 + prof + casting-mod, mirroring enginespell_save_dc; omitted — not faked — for a non-caster class), attack flag, damage dice/type, V/S/M components, material, upcast text, description.SpellsTabcards show a compact rules block (range / cast / duration / save DC / damage); the Spellbook browser shows the full block + description + "at higher levels". An SRD-miss spell shows just its name (today's behavior).4. Item Properties field was blank (optimizer)
_inventory_itemsnow surfaces the realitemcatalog.resolvestat block it was previously dropping — damage dice + type (weapons), base AC (armor/shields), SRDkind/category, attunement, and weapon/attunement property chips — alongside the existing weight/value/rarity backfill.screen-inventory.jsxItemDetail): Properties is no longer blank — renders Damage, Armor Class, attunement, SRD category, rarity, and property chips. (Removed the always-"Unknown" Origin / "—" Slot noise rows.) A free-text item the catalog can't resolve (e.g.Longsword +1,Healing Potion) shows weight/value only — honest, no fabricated damage.Read-model vs display, per bug (honest accounting)
proficient/expertisealready emitted)This matches the #272 triage note: the viewer read-models had the access but weren't emitting item damage/properties or spell rules.
Tests
+8 additive cases in
viewer/tests/test_readmodel_surfaces.py:Existing surface tests stay green (30 pass in
test_readmodel_surfaces.py; the full viewer suite is green locally except one pre-existingtest_portrait_gencase that needspydantic, unrelated to this change). Both JSX files transpile clean (Babel React preset). Heavy local tests deferred to CI in the cloud.Deferred
The 5th sweep finding — no level-up / ASI / subclass-choice screen — is a larger new interactive flow (needs an engine write-lane + multi-step modal), so it is filed as a follow-up, not built here: #397.
Do NOT close on merge — verify on the next 5-persona sweep (optimizer/veteran ≥7).
Summary by CodeRabbit
Bug Fixes
Tests