-
Notifications
You must be signed in to change notification settings - Fork 0
feat(viewer): 4 sat→7 web-viewer UX/correctness fixes (battle-log sanitize, dice-header strip, roll context, drop-confirm) #1161
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
base: main
Are you sure you want to change the base?
Changes from 2 commits
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -103,6 +103,19 @@ function ScreenInventory({ onNavigate, state, setState }) { | |
| }, [loadSurface]); | ||
|
|
||
| const hero = party.find((p) => p.id === activeHero) || party[0] || null; | ||
| // SAT→7 (drop-confirm on equipped gear): a one-click Drop on WORN gear (body armor especially) is | ||
| // irreversible with no safety net. This wrapper performs the drop /move unchanged for a LOOSE stash | ||
| // item (no nag), but gates an EQUIPPED item behind window.confirm (the same pattern as quickload / | ||
| // seed-reset). Cancel = no-op; confirm = the exact original drop move. Shared by both Drop affordances | ||
| // (context-menu + detail pane) so they nag identically. Loose inventory is never nagged. | ||
| const confirmDrop = React.useCallback((item, doDrop) => { | ||
| if (typeof doDrop !== "function" || !item) return; | ||
| if (isItemEquipped(item, hero && hero.equipped)) { | ||
| const ok = window.confirm(dropEquippedConfirmMessage(item.name)); | ||
| if (!ok) return; | ||
| } | ||
| doDrop(); | ||
| }, [hero]); | ||
| // RRI-5e98e6f: derive the displayed coin purse from the ONE shared selector + normalizer the | ||
| // Market also uses, so the same character's coins read identically on both screens (no | ||
| // 35-vs-232 divergence). partyPurse resolves the active hero (else party[0]) and zeroes/ints it. | ||
|
|
@@ -361,7 +374,7 @@ function ScreenInventory({ onNavigate, state, setState }) { | |
| : { label: "Hand to a companion (preview)", icon: "→", disabled: true, title: "Display-only — start a live session to act", onClick: () => toast({ kind: "item", title: ctxMenu.item.name + " handed over" }) }, | ||
| { divider: true }, | ||
| canAct | ||
| ? { label: "Drop", icon: "▾", tone: "crimson", title: "Relays to the DM via /move — the engine resolves it", onClick: () => postInvMove("do", { text: "I drop the " + ctxMenu.item.name + "." }, { kind: "danger", title: "Dropping " + ctxMenu.item.name, body: "Relayed to the DM — you will not get it back unless you fetch it yourself." }) } | ||
| ? { label: "Drop", icon: "▾", tone: "crimson", title: "Relays to the DM via /move — the engine resolves it", onClick: () => confirmDrop(ctxMenu.item, () => postInvMove("do", { text: "I drop the " + ctxMenu.item.name + "." }, { kind: "danger", title: "Dropping " + ctxMenu.item.name, body: "Relayed to the DM — you will not get it back unless you fetch it yourself." })) } | ||
| : { label: "Drop (preview)", icon: "▾", tone: "crimson", disabled: true, title: "Display-only — start a live session to act", onClick: () => toast({ kind: "danger", title: "Dropped: " + ctxMenu.item.name, body: "You will not get it back unless you fetch it yourself." }) }, | ||
| ]} | ||
| /> | ||
|
|
@@ -416,6 +429,27 @@ function inferEquipSlotId(name) { | |
| return ""; // unrecognized — placed into the first free generic cell by assignEquipSlots | ||
| } | ||
|
|
||
| // SAT→7 (adversarial minor): is `item` one of the hero's CURRENTLY-EQUIPPED pieces? Equipped gear is | ||
| // hero.equipped (the worn read-model). We match on the name SLUG so a display-name vs. catalog-name | ||
| // nuance ("Studded Leather" vs "studded leather armor"…) doesn't miss — but stay STRICT (exact slug) | ||
| // so a loose stash item is never wrongly flagged. Pure: reads only its args, writes nothing. | ||
| function isItemEquipped(item, equipped) { | ||
| if (!item || !item.name || !Array.isArray(equipped)) return false; | ||
| const target = slug(item.name); | ||
| if (!target) return false; | ||
| return equipped.some((e) => e && e.name && slug(e.name) === target); | ||
| } | ||
| // The confirm copy for dropping/unequipping a WORN piece — irreversible, body armor especially. One | ||
| // shared string so the context-menu Drop and the detail-pane Drop nag identically. Returns the message; | ||
| // callers gate the actual drop behind `window.confirm(this)` (matching screen-settings/screen-seed). | ||
| function dropEquippedConfirmMessage(name) { | ||
| return ( | ||
| "Drop " + (name || "this item") + "?\n\n" + | ||
| "It is CURRENTLY EQUIPPED. Dropping it unequips and discards it — you will not get it back unless " + | ||
| "you fetch it yourself. This cannot be undone." | ||
| ); | ||
| } | ||
|
|
||
| // Build a { slotId: equippedItem } map from hero.equipped. Rings and weapons spill from | ||
| // their primary cell to the secondary (ring1→ring2, mainhand→offhand) so a hero wearing two | ||
| // rings or dual-wielding shows both. Anything unrecognized lands in the first open cell so | ||
|
|
@@ -853,7 +887,12 @@ function ItemDetail({ item, hero, toast, canAct, postInvMove, examineSignal }) { | |
| )} | ||
| <BrassButton tone="ghost" size="sm" aria-haspopup="dialog" onClick={() => setExamineOpen(true)}>Examine</BrassButton> | ||
| {canAct ? ( | ||
| <BrassButton tone="ghost" size="sm" title="Relays to the DM via /move — the engine resolves it" onClick={() => postInvMove("do", { text: "I drop the " + item.name + "." }, { kind: "danger", title: "Dropping " + item.name, body: "Relayed to the DM." })}> | ||
| <BrassButton tone="ghost" size="sm" title="Relays to the DM via /move — the engine resolves it" onClick={() => { | ||
| // SAT→7: confirm before dropping CURRENTLY-EQUIPPED gear (worn body armor especially) — loose | ||
| // stash items drop with no nag. Mirrors the context-menu Drop via the same shared helpers. | ||
| if (isItemEquipped(item, hero && hero.equipped) && !window.confirm(dropEquippedConfirmMessage(item.name))) return; | ||
|
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. P3: Drop-confirm guard is duplicated inline in ItemDetail instead of reusing confirmDrop The context-menu Drop path (line 377) routes through the shared Why this matters: The PR's stated invariant is 'shared by both Drop affordances so they nag identically' — that invariant currently holds by hand-synchronization, not by code structure, and is the kind of divergence that a future edit will silently break on one path only. |
||
| postInvMove("do", { text: "I drop the " + item.name + "." }, { kind: "danger", title: "Dropping " + item.name, body: "Relayed to the DM." }); | ||
| }}> | ||
| Drop | ||
| </BrassButton> | ||
| ) : ( | ||
|
|
@@ -890,4 +929,4 @@ function itemCategory(item) { | |
|
|
||
| function toRoman(n) { return ["", "I", "II", "III", "IV", "V"][n] || n; } | ||
|
|
||
| Object.assign(window, { ScreenInventory, CoinSlot, ItemSlot, ItemDetail, packContents, EQUIP_SLOTS, PaperDoll, EquipSlotCell, inferEquipSlotId, assignEquipSlots, ITEM_TYPES, ITEM_KINDS, itemCategory, toRoman, slug, itemScope, itemStatRows, itemCompareRows }); | ||
| Object.assign(window, { ScreenInventory, CoinSlot, ItemSlot, ItemDetail, packContents, EQUIP_SLOTS, PaperDoll, EquipSlotCell, inferEquipSlotId, assignEquipSlots, isItemEquipped, dropEquippedConfirmMessage, ITEM_TYPES, ITEM_KINDS, itemCategory, toRoman, slug, itemScope, itemStatRows, itemCompareRows }); | ||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -65,13 +65,21 @@ const _BARE_TOOL_LINE = new RegExp( | |
| "^\\s*[`'\"(_*]*\\s*(?:" + _TOOLS_ALT + ")\\s*(?:\\([^)]*\\))?\\s*[`'\")_*]*\\s*[.;:]?\\s*$", | ||
| "i", | ||
| ); | ||
| // SAT→7 (narrative friction): the DM agent occasionally emits a Markdown HORIZONTAL RULE as a | ||
| // section separator ("---", "***", "___", or "— —") on its OWN line — a bare structural divider | ||
| // that reads as a stray glyph in the player's story scroll. A line that is NOTHING BUT a rule | ||
| // (3+ of the same rule char, optionally spaced) is formatting, never fiction, so drop it whole. | ||
| // HIGH-CONFIDENCE: requires the whole line to be the rule (an em-dash "—pause—" mid-prose, or a | ||
| // real "wait — what?" with surrounding words, is NOT a bare rule line and is untouched). | ||
| const _HRULE_LINE = /^\s*(?:-\s*){3,}\s*$|^\s*(?:\*\s*){3,}\s*$|^\s*(?:_\s*){3,}\s*$|^\s*(?:—\s*){2,}\s*$/; | ||
|
coderabbitai[bot] marked this conversation as resolved.
|
||
| function _isInternalLine(line) { | ||
| const t = (line || "").trim(); | ||
| if (!t) return false; // keep blank lines for caller's join (they're harmless) | ||
| return _GM_ADVISORY_HEADER.test(t) | ||
| || _ADVISORY_SUBTITLE.test(t) | ||
| || _ADVISORY_DIRECTIVE.test(t) | ||
| || _BARE_TOOL_LINE.test(t); | ||
| || _BARE_TOOL_LINE.test(t) | ||
| || _HRULE_LINE.test(t); | ||
| } | ||
|
|
||
| // #347: the SECOND class of DM-internal leak — the DM agent's STORY-CRAFT scaffolding | ||
|
|
@@ -125,6 +133,45 @@ const _STAGE_DIRECTION = new RegExp( | |
| "\\bof the\\b[^.]{0,30}\\b(?:cold open|beat|act|scene)\\b[^.]{0,30}\\bcomplete\\b" + | ||
| ")", "i", | ||
| ); | ||
| // (3b) SAT→7 (narrative's only major — this "cracked the 9/10"): the ROLL-RESULT-SUMMARY HEADER. | ||
| // Before narrating an outcome the DM agent sometimes states the raw check totals as a header line — | ||
| // "The intimidation lands at 18; the quiet interpose at 16." — bookkeeping that belongs in the dice | ||
| // tray, not the story prose. The fingerprint is a check NOUN/result "lands at <N>" optionally chained | ||
| // with one or more "; <thing> at <M>" clauses, all on bare INTEGER roll totals. Drop the whole sentence. | ||
| // | ||
| // HIGH-CONFIDENCE (story quality is the north star — never eat real prose): the trigger REQUIRES a bare | ||
| // integer immediately after "at" (a roll total, e.g. "at 18") AND a roll-summary VERB ("lands at"). The | ||
| // canonical verb is what disambiguates a check total from ordinary fiction: "the arrow lands at his feet" | ||
| // (noun, not a number) and "we arrive at the gate" (no roll verb) both pass through untouched, and a bare | ||
| // "the bell strikes at 12." / "we meet at 3." (no roll verb) is NEVER matched. This is the PRIMARY arm — | ||
| // "<thing> lands/comes-in/settles/resolves/clears at <N>". | ||
| // | ||
| // CRITICAL refinement (the false-positive the verb anchor alone did NOT catch): the integer must be a | ||
| // roll TOTAL, which always ends the clause — a check result is "lands at 18." / "lands at 18;", NEVER | ||
| // "lands at 18 men". So the bare "at <N>" is only a roll summary when <N> is CLAUSE-TERMINAL: immediately | ||
| // followed by clause-ending punctuation ([.;,!?]) or end-of-string (the "; <thing> at <M>" chained- | ||
| // continuation case is just a ';'-terminal primary). When <N> is followed by a WORD/noun it is an in-world | ||
| // quantity — "the line falls at 12 men", "settles at 5 gold", "comes in at 40 pounds", "clears at 6 bells" | ||
| // — genuine fiction that MUST survive verbatim. `_ROLL_VERB_AT_N` is the shared, clause-terminal core. | ||
| const _ROLL_VERB_AT_N = | ||
| "\\b(?:lands?|comes?(?:\\s+in)?|falls?|settles?|resolves?|clears?)\\s+(?:in\\s+)?at\\s+\\d+(?=[.;,!?]|\\s*$)"; | ||
|
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. P2: Roll-summary regex has a narrow theoretical false-positive on terminal " at ." prose
Why this matters: Silent deletion of real narration is a story-quality regression that is hard to spot in QA; tightening the subject anchor removes the only remaining false-positive vector the verb+number guard still admits. |
||
| const _ROLL_RESULT_SUMMARY = new RegExp(_ROLL_VERB_AT_N, "i"); | ||
| // The FULL roll-summary HEADER LINE — the verb-anchored primary PLUS any "; <thing> at <N>" continuations | ||
| // chained onto it ("The intimidation lands at 18; the quiet interpose at 16."). This is matched at the LINE | ||
| // level (before the per-sentence splitter fragments it on the ";"), so the whole header — including its | ||
| // trailing clauses and an optional trailing rule — is dropped as a unit. It REQUIRES the leading roll verb, | ||
| // so it can only extend a genuine roll-summary; a standalone "; we meet at 3" never has the primary verb and | ||
| // is never reached. Anchored to the whole line ('^…$') so it only fires on a header, not a mid-prose clause. | ||
| // Each "at <N>" (primary AND every chained continuation) is clause-terminal — the same total-ends-the-clause | ||
| // rule as `_ROLL_VERB_AT_N` — so a header whose number is followed by a noun ("falls at 12 men …") never | ||
| // matches and the genuine-fiction line survives whole. | ||
| const _ROLL_SUMMARY_HEADER_LINE = new RegExp( | ||
| "^\\s*[^.!?]*?" + | ||
| _ROLL_VERB_AT_N + | ||
| "(?:[^.!?]*?\\bat\\s+\\d+(?=[.;,!?]|\\s*$))*" + | ||
|
Comment on lines
+156
to
+171
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. 🎯 Functional Correctness | 🟠 Major 🧩 Analysis chain🏁 Script executed: #!/bin/bash
# Look for roll-summary header formatting in engine/prompt sources to confirm separator style.
rg -nP -C2 '(lands?|comes?|falls?|settles?|resolves?|clears?)\s+(in\s+)?at\s+\d+' -g '!viewer/tests/**'Repository: electricsheephq/WorldOS Length of output: 161 🏁 Script executed: # Verify the file and code at the specified lines
cat -n viewer/openworlds/screen-table.jsx | sed -n '156,171p'Repository: electricsheephq/WorldOS Length of output: 1535 🏁 Script executed: # Find test files related to roll scaffolding and check for comma-terminal tests
fd -e 'jsx' -e 'js' -e 'test' -e 'spec' . viewer/tests/ | head -20Repository: electricsheephq/WorldOS Length of output: 161 🏁 Script executed: # Search for uses of _isScaffoldingSentence and _hasScaffolding to understand how the regex is used
rg -n '_isScaffoldingSentence|_hasScaffolding' viewer/openworlds/Repository: electricsheephq/WorldOS Length of output: 643 🏁 Script executed: # Look for test cases that explicitly test the roll verb patterns
rg -n '_ROLL_VERB_AT_N|_ROLL_RESULT_SUMMARY|_ROLL_SUMMARY_HEADER_LINE' -A5 -B5 viewer/tests/Repository: electricsheephq/WorldOS Length of output: 161 🏁 Script executed: # View the _isScaffoldingSentence function to understand how _ROLL_RESULT_SUMMARY is used
sed -n '262,280p' viewer/openworlds/screen-table.jsxRepository: electricsheephq/WorldOS Length of output: 1285 🏁 Script executed: # View the _hasScaffolding function
sed -n '268,275p' viewer/openworlds/screen-table.jsxRepository: electricsheephq/WorldOS Length of output: 675 🏁 Script executed: # Find test files - may be in different locations
find viewer -type f \( -name '*.test.js' -o -name '*.test.jsx' -o -name '*.spec.js' -o -name '*test*.js' \)Repository: electricsheephq/WorldOS Length of output: 161 🏁 Script executed: # Search more broadly for test-related patterns including "test_roll_verb" mentioned in review
rg -n 'test_roll_verb|roll.*survives|scaffolding' viewer/ --type js --type jsxRepository: electricsheephq/WorldOS Length of output: 189 🏁 Script executed: # Search for test files more broadly without -t flag
find viewer -type f -name '*test*' -o -name '*spec*' 2>/dev/null | head -30Repository: electricsheephq/WorldOS Length of output: 1330 🏁 Script executed: # Search for the specific test mentioned in review: test_roll_verb_with_in_world_quantity_survives_verbatim
rg -n 'test_roll_verb_with_in_world_quantity_survives_verbatim|in_world_quantity' viewer/Repository: electricsheephq/WorldOS Length of output: 276 🏁 Script executed: # Look at the full _stripScaffoldingSentences function to understand the code path
sed -n '276,310p' viewer/openworlds/screen-table.jsxRepository: electricsheephq/WorldOS Length of output: 2286 🏁 Script executed: # View the test that reviewer mentioned
sed -n '370,395p' viewer/tests/test_sanitize_narration.pyRepository: electricsheephq/WorldOS Length of output: 2111 🏁 Script executed: # Look for other roll-verb related tests to see what terminals they use
rg -n 'falls.*at|lands.*at|settles.*at|resolves.*at' viewer/tests/test_sanitize_narration.py -A2 -B2Repository: electricsheephq/WorldOS Length of output: 5017 Remove comma from roll-verb terminal lookahead in both Root cause: The unanchored Code path: Line 293 filters sentence fragments with Impact: Sentences like "The hawk settles at 3, then takes flight." or "The price falls at 5, and the crowd cheers." are erased from player-facing narration. Test The fix (remove Proposed fix const _ROLL_VERB_AT_N =
- "\\b(?:lands?|comes?(?:\\s+in)?|falls?|settles?|resolves?|clears?)\\s+(?:in\\s+)?at\\s+\\d+(?=[.;,!?]|\\s*$)";
+ "\\b(?:lands?|comes?(?:\\s+in)?|falls?|settles?|resolves?|clears?)\\s+(?:in\\s+)?at\\s+\\d+(?=[.;!?]|\\s*$)";
@@
const _ROLL_SUMMARY_HEADER_LINE = new RegExp(
"^\\s*[^.!?]*?" +
_ROLL_VERB_AT_N +
- "(?:[^.!?]*?\\bat\\s+\\d+(?=[.;,!?]|\\s*$))*" +
+ "(?:[^.!?]*?\\bat\\s+\\d+(?=[.;!?]|\\s*$))*" +🧰 Tools🪛 ast-grep (0.44.0)[warning] 157-157: Detects non-literal values in regular expressions (detect-non-literal-regexp) 🤖 Prompt for AI AgentsSource: Linters/SAST tools |
||
| "\\s*[.!?]?\\s*(?:-{3,}|\\*{3,}|_{3,}|—{2,})?\\s*$", | ||
| "i", | ||
| ); | ||
| // (4) #752: INTER-BEAT TRANSITION meta-text — the engine/DM "seam" stage-directions that leaked | ||
| // into the chronicle BETWEEN player beats (adversarial confirm sweep: "Engine META-TEXT transition | ||
| // phrases leak into the chronicle between player beats"). These announce the move from one beat/ | ||
|
|
@@ -216,22 +263,28 @@ function _isScaffoldingSentence(sentence) { | |
| const s = (sentence || "").trim(); | ||
| if (!s) return false; | ||
| return _CRAFT_JARGON.test(s) || _STAGE_DIRECTION.test(s) || _BEAT_TRANSITION.test(s) | ||
| || _AUTHORING_PREAMBLE.test(s); | ||
| || _AUTHORING_PREAMBLE.test(s) || _ROLL_RESULT_SUMMARY.test(s); | ||
| } | ||
| function _hasScaffolding(text) { | ||
| return _TALLY_TEST.test(text) || _CRAFT_JARGON.test(text) | ||
| || _STAGE_DIRECTION.test(text) || _BEAT_TRANSITION.test(text) | ||
| || _AUTHORING_PREAMBLE.test(text); | ||
| || _AUTHORING_PREAMBLE.test(text) || _ROLL_RESULT_SUMMARY.test(text); | ||
| } | ||
| // Clean scaffolding from one line, preserving the real prose. Two grains: | ||
| // Clean scaffolding from one line, preserving the real prose. Three grains: | ||
| // 0. drop a WHOLE roll-summary HEADER line (verb-anchored "lands at N" + its "; … at M" chain) — done | ||
| // before the splitter so the chained clauses don't get peeled apart and survive piecemeal; | ||
| // 1. excise dice/check TALLY phrases in place (keeps the rest of their sentence); | ||
| // 2. split into sentences (on . ! ? ;, terminator kept) and DROP any sentence that is | ||
| // wholly plot-craft jargon / a stage-direction. | ||
| // wholly plot-craft jargon / a stage-direction / a (single-clause) roll-summary. | ||
| // Returns the cleaned line (possibly ""). | ||
| function _stripScaffoldingSentences(line) { | ||
| if (typeof line !== "string" || !line) return line; | ||
| // Fast path: nothing scaffolding-shaped here, don't touch the line at all. | ||
| if (!_hasScaffolding(line)) return line; | ||
| // (0) A line that is WHOLLY a roll-summary header (the verb-anchored primary plus any chained "; … at N" | ||
| // continuations, modulo a trailing rule) is bookkeeping through-and-through — drop it entirely so a | ||
| // chained second clause ("…; the quiet interpose at 16.") can't ride through after the ";"-splitter. | ||
| if (_ROLL_SUMMARY_HEADER_LINE.test(line)) return ""; | ||
| // (1) In-place tally excision (global; reset lastIndex defensively). | ||
| _TALLY.lastIndex = 0; | ||
| let cleaned = line.replace(_TALLY, ""); | ||
|
|
@@ -240,10 +293,15 @@ function _stripScaffoldingSentences(line) { | |
| if (parts) cleaned = parts.filter((p) => !_isScaffoldingSentence(p)).join(""); | ||
| // Tidy punctuation/space the excisions may have left (" ." / " " / leading "; " / | ||
| // a clause separator now dangling at the end, e.g. "Zevlor held silence;" → "…silence."). | ||
| // SAT→7: also peel a TRAILING horizontal-rule glyph the DM tacked after a roll-summary header | ||
| // ("…at 16. ---") so the divider doesn't linger once its summary line is gone. Only a trailing | ||
| // run of pure rule chars is removed; an em-dash bound to words ("groans open —") is left intact | ||
| // because the rule run must be ≥3 dashes / ≥2 em-dashes (a bare divider, never inline prose). | ||
| return cleaned | ||
| .replace(/\s+([.!?;,])/g, "$1") | ||
| .replace(/\s{2,}/g, " ") | ||
| .replace(/^[\s;,.]+/, "") | ||
| .replace(/\s*(?:-{3,}|\*{3,}|_{3,}|—{2,})\s*$/, "") | ||
| .replace(/\s*[;,]\s*$/, ".") | ||
| .trim(); | ||
| } | ||
|
|
@@ -276,6 +334,27 @@ if (typeof window !== "undefined") { | |
| window.WRAPPER_PROGRESS_LINES = Array.from(_WRAPPER_PROGRESS_LINES); | ||
| window.isWrapperProgressLine = _isWrapperProgressLine; | ||
| } | ||
| // SAT→7 (die-roll button context): pure builder for the dice-button move + its chronicle echo, so the | ||
| // "never fire a contextless roll" guard is unit-testable without mounting ScreenTable. Mirrors the | ||
| // Declare text-guard: a TYPED intent is the move's reason; absent one, the latest narrative line is | ||
| // attached so the DM resolves the check against the moment in play. Returns { move, echo } where `move` | ||
| // is the POST payload (kind/name/text) and `echo` is the verb PHRASE recordPlayerEcho prepends the | ||
| // hero name to. `intent`/`context` are trimmed defensively; no name is baked into the echo. | ||
| function composeRollMove(sides, intent, context) { | ||
| const n = Number(sides) || 20; | ||
| const want = (typeof intent === "string" ? intent : "").trim(); | ||
| const ctx = (typeof context === "string" ? context : "").trim(); | ||
| const text = want | ||
| ? `I roll a d${n} to ${want}` | ||
| : ctx | ||
| ? `I roll a d${n} (in response to: ${ctx})` | ||
| : `I roll a d${n}`; | ||
| const echo = want ? `rolls a d${n} to ${want}` : `rolls a d${n}`; | ||
| return { move: { kind: "check", name: `d${n}`, text }, echo }; | ||
|
Comment on lines
+347
to
+353
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. 🎯 Functional Correctness | 🟡 Minor | ⚡ Quick win Include fallback context in the optimistic echo. Confidence: 88%. The fallback path puts Proposed fix- const echo = want ? `rolls a d${n} to ${want}` : `rolls a d${n}`;
+ const echo = want
+ ? `rolls a d${n} to ${want}`
+ : ctx
+ ? `rolls a d${n} in response to: ${ctx}`
+ : `rolls a d${n}`;Also applies to: 1289-1292 🤖 Prompt for AI Agents |
||
| } | ||
| if (typeof window !== "undefined") { | ||
| window.composeRollMove = composeRollMove; | ||
| } | ||
| function sanitizeNarration(text) { | ||
| if (typeof text !== "string" || !text) return ""; | ||
| const kept = text | ||
|
|
@@ -1205,13 +1284,23 @@ function ScreenTable({ onNavigate, state, setState, liveSession }) { | |
| ? "Type a move before declaring" | ||
| : "Declare move"; | ||
|
|
||
| // SAT→7 (adversarial minor): the dice buttons used to fire a CONTEXTLESS "requests a d20 roll" — | ||
| // they bypassed the Declare guard entirely, so the DM got a bare die with no idea WHAT the hero is | ||
| // attempting. Parity with the Declare text-guard: prefer the intent the player TYPED into the composer | ||
| // (so "d20 to pick the lock" reaches the engine); otherwise ATTACH the latest narrative context (the | ||
| // newest sanitized scene line) so the roll resolves against the moment in play instead of nothing. | ||
| // Either way the move carries a reason, and the echo the player sees names it — never a bare "d20 roll". | ||
| const requestRoll = (sides = 20) => { | ||
| const action = actionById("check"); | ||
| if (!action?.available) { | ||
| toast({ kind: "danger", title: `d${sides} unavailable`, body: action?.disabled_reason || readOnlyReason }); | ||
| return; | ||
| } | ||
| postMove({ kind: "check", name: `d${sides}`, text: `roll d${sides}` }, `requests a d${sides} roll`, "check"); | ||
| // Build the contextful move + echo via the pure helper (typed intent first, else the latest scene | ||
| // line). recordPlayerEcho prepends hero.name to the verb-phrase echo, so it never reads as a bare die. | ||
| const { move, echo } = composeRollMove(sides, draftText, latestStreamedLine); | ||
| postMove(move, echo, "check"); | ||
| if (draftText) setInput(""); | ||
| }; | ||
|
|
||
| const invokeAction = (action) => { | ||
|
|
||
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
🎯 Functional Correctness | 🟡 Minor | ⚡ Quick win
Name-based equip detection will false-positive on duplicate item names
Lines 436-440 use slugged-name equality as the equip identity. If a hero has one equipped copy and one loose copy with the same name, dropping the loose copy still triggers the equipped confirm path. That breaks the “confirm only on currently equipped items” contract in both Drop flows.
Use stable item identity (
id/instance key) first, and only fall back to name matching when no identity exists on either side. Also add a regression test for “equipped + loose same-name item” to lock this behavior.Confidence: 95%.
Proposed fix (ID-first matching)
function isItemEquipped(item, equipped) { - if (!item || !item.name || !Array.isArray(equipped)) return false; - const target = slug(item.name); - if (!target) return false; - return equipped.some((e) => e && e.name && slug(e.name) === target); + if (!item || !Array.isArray(equipped)) return false; + + const itemId = item.id ?? item.item_id ?? null; + if (itemId !== null) { + return equipped.some((e) => e && (e.id === itemId || e.item_id === itemId)); + } + + if (!item.name) return false; + const target = slug(item.name); + if (!target) return false; + return equipped.some((e) => { + if (!e || !e.name) return false; + const equippedId = e.id ?? e.item_id ?? null; + return equippedId === null && slug(e.name) === target; + }); }🤖 Prompt for AI Agents