From 56164e85f0b5b5fe4dacac972a4f47b218ca5685 Mon Sep 17 00:00:00 2001 From: lgray Date: Tue, 4 Aug 2026 22:33:47 -0500 Subject: [PATCH 1/9] feat(client): replace the PayAmountChoice slider with a sanitized numeric box MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit The amount prompts drove a `` whose value could only ever be in range, so the client never had to sanitize anything. A range control is also a poor fit for a 0..1000 LoopCollapse window: naming an exact N by dragging is impractical, and the slider announced its bounds only through native semantics that a text box does not inherit. `AmountInput` is the shared replacement for all three amount prompts (PayAmountChoice / ChooseXValue / AssistPayment); this commit lands the control and its first adopter. The `[min, max]` window is ENGINE-OWNED and arrives as props — the component holds no bound, no default and no fallback of its own. `parseAmount` is the single sanitization authority and REJECTS rather than coerces, so a player never submits a number they did not type. Digits-only is load-bearing: `Number()` alone accepts `""`→0, `" 7 "`→7, `"1.5"`→1.5, `"1e3"`→1000, `"+2"`→2 and `"0x10"`→16, every one of which lands inside a typical window. The commit button and the Enter key share one guard, in `handleCommit`. Accessibility keeps everything the slider exposed: the box and both steppers carry accessible names, the `[min,max]` hint is permanently associated via `aria-describedby` (a text box announces no bounds natively), the validation message is appended to that association while `aria-invalid` is set, and ArrowUp/ArrowDown step. Only the pointer-drag affordance is gone. The steppers stay LIVE while the entry is invalid — with the slider deleted they are the only non-typing way back into the window — and they step from the DIGIT reading of the raw entry, so 1001 recovers to 1000 rather than collapsing to min. The three new `mana.*` keys land here rather than with the rest of the i18n ledger because `react-i18next.d.ts` binds the English catalog as the type oracle: `t("mana.amountOutOfRange")` is a compile error until the key exists, so scheduling it later would leave this commit unable to type-check at its own boundary. All 7 locales move together, so per-locale key parity holds here. `PayAmountChoiceWaitingForFactory` replaces the hand-rolled `WaitingFor` literal in the test file, mirroring the ChooseXValue factory pair. Assisted-by: ClaudeCode:claude-opus-5 --- client/src/components/mana/AmountInput.tsx | 156 +++++++++++++ .../src/components/mana/PayAmountChoiceUI.tsx | 62 +++--- .../mana/__tests__/AmountInput.test.tsx | 202 +++++++++++++++++ .../mana/__tests__/PayAmountChoiceUI.test.tsx | 205 +++++++++++++++++- client/src/i18n/locales/de/game.json | 3 + client/src/i18n/locales/en/game.json | 3 + client/src/i18n/locales/es/game.json | 3 + client/src/i18n/locales/fr/game.json | 3 + client/src/i18n/locales/it/game.json | 3 + client/src/i18n/locales/pl/game.json | 3 + client/src/i18n/locales/pt/game.json | 3 + client/src/test/factories/gameStateFactory.ts | 29 +++ 12 files changed, 642 insertions(+), 33 deletions(-) create mode 100644 client/src/components/mana/AmountInput.tsx create mode 100644 client/src/components/mana/__tests__/AmountInput.test.tsx diff --git a/client/src/components/mana/AmountInput.tsx b/client/src/components/mana/AmountInput.tsx new file mode 100644 index 0000000000..e30db639c4 --- /dev/null +++ b/client/src/components/mana/AmountInput.tsx @@ -0,0 +1,156 @@ +import { useId } from "react"; +import { useTranslation } from "react-i18next"; + +import { gameButtonClass } from "../ui/buttonStyles.ts"; + +/** + * Shared bounded-amount control for the engine's amount prompts + * (PayAmountChoice / ChooseXValue / AssistPayment). + * + * The `[min, max]` window is ENGINE-OWNED and arrives as props — this component holds no bound of + * its own, no default, and no fallback. `parseAmount` is the single sanitization authority; it + * REJECTS (returns null) rather than coercing, so a player never submits a number they did not type. + */ + +/** Digit reading of `raw`, ignoring the window. Recovery uses this; SUBMISSION uses `parseAmount`. */ +function digitsOf(raw: string): number | null { + return /^\d+$/.test(raw) ? Number(raw) : null; +} + +export function parseAmount(raw: string, min: number, max: number): number | null { + // Digits only. `Number()` alone is NOT sufficient: MEASURED, Number("") === 0, + // Number(" 7 ") === 7, Number("1.5") === 1.5, Number("1e3") === 1000, Number("+2") === 2 and + // Number("0x10") === 16 all land INSIDE a typical window. + const value = digitsOf(raw); + return value !== null && value >= min && value <= max ? value : null; +} + +export interface AmountInputLabels { + /** aria-label for the numeric text box. */ + input: string; + /** aria-label for the − stepper. */ + decrease: string; + /** aria-label for the + stepper. */ + increase: string; +} + +export function AmountInput({ + raw, + onRawChange, + min, + max, + onSubmit, + labels, +}: { + raw: string; + onRawChange: (raw: string) => void; + min: number; + max: number; + /** Called on Enter. MUST itself reject an invalid amount — AmountInput deliberately does not + * re-guard, because a second guard would make the caller's guard unobservable and untestable. */ + onSubmit: () => void; + labels: AmountInputLabels; +}) { + const { t } = useTranslation("game"); + const amount = parseAmount(raw, min, max); + const hintId = useId(); + const errorId = useId(); + + // Recovery anchor. With the slider deleted the steppers are the only non-typing way out of an + // invalid entry, so they stay LIVE while `amount === null` and snap back into [min, max]. They + // step from the DIGIT reading, not from `amount`: `parseAmount` collapses "junk" and "out of + // range" into the same null, so stepping from `amount ?? min` would throw away a perfectly + // readable 1001 and jump to min. `parseAmount` gates SUBMISSION; `step` performs RECOVERY + // toward the window. + const step = (delta: number) => + onRawChange(String(Math.min(Math.max((digitsOf(raw) ?? min) + delta, min), max))); + const decDisabled = amount !== null && amount <= min; + const incDisabled = amount !== null && amount >= max; + + // ponytail: no showSlider/showSteppers flag — the slider is deleted, not configurable. + // ponytail: no role="alert" — assertive per-keystroke announcements are the anti-pattern; + // aria-invalid + aria-describedby is the association. + // ponytail: no pattern="[0-9]*" — inputMode="numeric" carries the modern-iOS keypad; add back + // only on a legacy-iOS report. + // ponytail: the null-guard lives once, in the caller's handleCommit — a second guard in + // onKeyDown would make it unobservable. + // ponytail: no role="spinbutton" — zero in-repo precedent, and it forces aria-valuenow/min/max + // upkeep. + return ( +
+
+ + onRawChange(e.target.value)} + onKeyDown={(e) => { + if (e.key === "Enter") { + e.preventDefault(); + onSubmit(); + return; + } + // type="text" has no native stepping; the accessibility floor requires arrows to step. + if (e.key === "ArrowUp") { + e.preventDefault(); + step(1); + } else if (e.key === "ArrowDown") { + e.preventDefault(); + step(-1); + } + }} + aria-label={labels.input} + aria-invalid={amount === null} + // The window is ANNOUNCED, not merely displayed. `type="range"` announced min/max/now + // natively; a text box announces nothing, so the range hint is permanently associated + // and the error message is appended to it while invalid. + aria-describedby={amount === null ? `${hintId} ${errorId}` : hintId} + // `w-24` not `w-20`: four digits must fit for the 1000 case. + className={`h-9 w-24 rounded-lg border bg-gray-950/80 px-2 text-center font-mono text-base font-semibold shadow-inner outline-none transition focus:ring-2 ${ + amount === null + ? "border-red-400/60 text-red-200 focus:ring-red-400/30" + : "border-cyan-400/30 text-cyan-100 focus:ring-cyan-400/30" + }`} + /> + + + {min > 0 ? t("mana.minMax", { min, max }) : t("mana.maxOnly", { max })} + +
+ + {amount === null && ( +

+ {t("mana.amountOutOfRange", { min, max })} +

+ )} +
+ ); +} diff --git a/client/src/components/mana/PayAmountChoiceUI.tsx b/client/src/components/mana/PayAmountChoiceUI.tsx index 53354f7e6a..c62e59bc7a 100644 --- a/client/src/components/mana/PayAmountChoiceUI.tsx +++ b/client/src/components/mana/PayAmountChoiceUI.tsx @@ -5,6 +5,7 @@ import { useTranslation } from "react-i18next"; import { useCanActForWaitingState } from "../../hooks/usePlayerId.ts"; import { useGameStore } from "../../stores/gameStore.ts"; import { gameButtonClass } from "../ui/buttonStyles.ts"; +import { AmountInput, parseAmount } from "./AmountInput.tsx"; export function PayAmountChoiceUI() { const { t } = useTranslation("game"); @@ -17,12 +18,14 @@ export function PayAmountChoiceUI() { const data = isPayAmount ? waitingFor.data : null; const min = data?.min ?? 0; const max = data?.max ?? 0; - const [value, setValue] = useState(min); + const [raw, setRaw] = useState(String(min)); useEffect(() => { - if (isPayAmount) setValue(min); + if (isPayAmount) setRaw(String(min)); }, [isPayAmount, min, max]); + const amount = parseAmount(raw, min, max); + const sourceName = useMemo(() => { if (!gameState || !data) return null; return gameState.objects[data.source_id]?.name ?? null; @@ -55,8 +58,12 @@ export function PayAmountChoiceUI() { data?.resource.type === "LoopCollapse" ? data.resource.data.axis : null; const handleCommit = useCallback(() => { - dispatch({ type: "SubmitPayAmount", data: { amount: value } }); - }, [dispatch, value]); + // Sanitization gate: an unparsed/out-of-range entry never reaches the engine. + // `amount === null`, NOT `!amount` — 0 is a legal amount on every PayAmountChoice minting + // site (all six hardcode `min: 0`). + if (amount === null) return; + dispatch({ type: "SubmitPayAmount", data: { amount } }); + }, [dispatch, amount]); if (!data || !canAct) return null; @@ -81,40 +88,41 @@ export function PayAmountChoiceUI() { )} -
- -
+
diff --git a/client/src/components/mana/__tests__/AmountInput.test.tsx b/client/src/components/mana/__tests__/AmountInput.test.tsx new file mode 100644 index 0000000000..50fadde731 --- /dev/null +++ b/client/src/components/mana/__tests__/AmountInput.test.tsx @@ -0,0 +1,202 @@ +import { afterEach, describe, expect, it, vi } from "vitest"; +import { cleanup, fireEvent, render, screen } from "@testing-library/react"; + +import { AmountInput, parseAmount } from "../AmountInput.tsx"; + +const LABELS = { + input: "Enter amount", + decrease: "Decrease amount", + increase: "Increase amount", +}; + +// Building-block level (CLAUDE.md "test the building block, not the special case"): every row +// below exercises `parseAmount`/`AmountInput` across their input range, not one card's prompt. +describe("parseAmount", () => { + // DISCRIMINATING rows — each is rejected by exactly one conjunct, named per row. + // `/^\d+$/` guard rows: `Number()` alone lands every one of these INSIDE the window. + it.each([ + ["", 0, 1000, 'Number("") === 0'], + [" 7 ", 0, 1000, 'Number(" 7 ") === 7'], + ["1.5", 0, 1000, 'Number("1.5") === 1.5'], + ["1e3", 0, 1000, 'Number("1e3") === 1000'], + ["+2", 0, 1000, 'Number("+2") === 2 — discriminates at min <= 2 ONLY'], + ["0x10", 0, 100, 'Number("0x10") === 16'], + ])( + "rejects %j in [%i, %i] via the digit guard (%s)", + (raw, min, max) => { + expect(parseAmount(raw as string, min as number, max as number)).toBeNull(); + }, + ); + + // DOMINATED rows — labelled so no later reader mistakes them for probes. `Number()` already + // yields NaN (or a negative that `min >= 0` rejects), so deleting `/^\d+$/` leaves them green. + it.each([ + ["12a", 0, 10, "NaN — DOMINATED"], + ["abc", 0, 10, "NaN — DOMINATED"], + ["Infinity", 0, 10, "Infinity > max — DOMINATED"], + ["1_0", 0, 10, "NaN — DOMINATED"], + ["-1", 0, 10, "min is u32-sourced so min >= 0 always — DOMINATED"], + ["+2", 5, 1000, "2 < min=5, so the range conjunct already rejects — DOMINATED at min=5"], + ])("rejects %j in [%i, %i] (%s)", (raw, min, max) => { + expect(parseAmount(raw as string, min as number, max as number)).toBeNull(); + }); + + // Range-conjunct rows. + it("rejects a value above max", () => { + expect(parseAmount("1001", 0, 1000)).toBeNull(); + }); + + it("rejects a value below min", () => { + expect(parseAmount("3", 5, 1000)).toBeNull(); + }); + + // ACCEPTING rows — the positive controls that make every rejection above non-vacuous. + it("accepts leading zeros as legal digits", () => { + expect(parseAmount("007", 0, 10)).toBe(7); + }); + + it("accepts max itself (the window is upper-INCLUSIVE)", () => { + expect(parseAmount("1000", 0, 1000)).toBe(1000); + }); + + it("accepts min itself (the window is lower-INCLUSIVE)", () => { + expect(parseAmount("5", 5, 1000)).toBe(5); + }); +}); + +describe("AmountInput — rendered contract", () => { + afterEach(() => { + cleanup(); + }); + + // No store reset: AmountInput is prop-driven and reads no store. `cleanup` IS mandatory — + // there is no global RTL auto-cleanup in this repo, so a second mount would make + // getByLabelText throw "Found multiple elements". + + it("A11Y/associate: announces the window and the validation message", () => { + const onRawChange = vi.fn(); + const { rerender } = render( + , + ); + + const box = screen.getByLabelText("Enter amount"); + expect(box).toHaveAttribute("aria-invalid", "true"); + + // Both ids are associated while invalid, and both RESOLVE to the elements that carry the + // hint and the validation copy — an id string alone would be a dangling reference. + const describedBy = box.getAttribute("aria-describedby")?.split(" ") ?? []; + expect(describedBy).toHaveLength(2); + const described = describedBy.map((id) => document.getElementById(id)); + expect(described.map((el) => el?.textContent)).toEqual([ + "max 5", + "Enter a whole number between 0 and 5", + ]); + + expect(screen.getByLabelText("Decrease amount")).toBeInTheDocument(); + expect(screen.getByLabelText("Increase amount")).toBeInTheDocument(); + + // A11Y/valid-state: the window stays ANNOUNCED once the entry is valid. This is the half + // that reverting to `amount === null ? errorId : undefined` would red. + rerender( + , + ); + const validBox = screen.getByLabelText("Enter amount"); + expect(validBox).toHaveAttribute("aria-invalid", "false"); + const validIds = validBox.getAttribute("aria-describedby")?.split(" ") ?? []; + expect(validIds).toHaveLength(1); + expect(document.getElementById(validIds[0])?.textContent).toBe("max 5"); + }); + + it("A11Y/arrows: ArrowUp/ArrowDown step and clamp to the window", () => { + const onRawChange = vi.fn(); + const { rerender } = render( + , + ); + + const box = screen.getByLabelText("Enter amount"); + fireEvent.keyDown(box, { key: "ArrowUp" }); + expect(onRawChange).toHaveBeenLastCalledWith("5"); + fireEvent.keyDown(box, { key: "ArrowDown" }); + expect(onRawChange).toHaveBeenLastCalledWith("3"); + + // Clamped, not 6 — the distinct probe for `Math.min(…, max)`. + rerender( + , + ); + fireEvent.keyDown(screen.getByLabelText("Enter amount"), { key: "ArrowUp" }); + expect(onRawChange).toHaveBeenLastCalledWith("5"); + }); + + it("N5/recover-junk: steppers stay LIVE on an unreadable entry and re-enter the window", () => { + const onRawChange = vi.fn(); + render( + , + ); + + // The recovery anchor: with the slider deleted these are the only non-typing way out. + expect(screen.getByLabelText("Decrease amount")).not.toBeDisabled(); + expect(screen.getByLabelText("Increase amount")).not.toBeDisabled(); + + fireEvent.click(screen.getByLabelText("Increase amount")); + expect(onRawChange).toHaveBeenLastCalledWith("1"); + fireEvent.click(screen.getByLabelText("Decrease amount")); + expect(onRawChange).toHaveBeenLastCalledWith("0"); + }); + + it("N5/recover-range: a READABLE out-of-range entry steps back to the bound, not to min", () => { + const onRawChange = vi.fn(); + render( + , + ); + + // THE N5 discriminator. `step` reads the DIGITS of raw, so 1001 recovers to 1000. Under the + // `(amount ?? min) + delta` implementation both of these would be "0"/"1" — and the "abc" + // case above stays green under BOTH, which is why the two cases are separate tests. + fireEvent.click(screen.getByLabelText("Decrease amount")); + expect(onRawChange).toHaveBeenLastCalledWith("1000"); + fireEvent.click(screen.getByLabelText("Increase amount")); + expect(onRawChange).toHaveBeenLastCalledWith("1000"); + }); +}); diff --git a/client/src/components/mana/__tests__/PayAmountChoiceUI.test.tsx b/client/src/components/mana/__tests__/PayAmountChoiceUI.test.tsx index 98e2d245d7..8dccca2d06 100644 --- a/client/src/components/mana/__tests__/PayAmountChoiceUI.test.tsx +++ b/client/src/components/mana/__tests__/PayAmountChoiceUI.test.tsx @@ -1,8 +1,11 @@ import { afterEach, beforeEach, describe, expect, it } from "vitest"; -import { cleanup, render, screen } from "@testing-library/react"; +import { cleanup, fireEvent, render, screen } from "@testing-library/react"; import type { LoopCollapseAxis, WaitingFor } from "../../../adapter/types"; -import { buildGameState } from "../../../test/factories/gameStateFactory.ts"; +import { + buildGameState, + buildPayAmountChoiceWaitingFor, +} from "../../../test/factories/gameStateFactory.ts"; import { setGameStoreForTest } from "../../../test/helpers/gameStoreHelpers.ts"; import { useGameStore } from "../../../stores/gameStore"; import { PayAmountChoiceUI } from "../PayAmountChoiceUI.tsx"; @@ -10,17 +13,16 @@ import { PayAmountChoiceUI } from "../PayAmountChoiceUI.tsx"; // CR 732.2a: the LoopCollapse prompt must name the axis the loop collapses. The // counter/life labels are iteration-framed (×N), never a raw token count. function loopCollapseWaitingFor(axis: LoopCollapseAxis, min = 0): WaitingFor { - return { - type: "PayAmountChoice", + return buildPayAmountChoiceWaitingFor({ data: { player: 0, resource: { type: "LoopCollapse", data: { axis } }, - // The stepper initializes `value` to `min`, so `min` pins the rendered count. + // The box initializes `raw` to `String(min)`, so `min` pins the rendered count. min, max: 1000, source_id: 0, }, - }; + }); } // player 0 == local PLAYER_ID, and turn_decision_controller/active_player are the @@ -87,3 +89,194 @@ describe("PayAmountChoiceUI — LoopCollapse axis label", () => { ).toBeInTheDocument(); }); }); + +const BOX = "Choose amount to pay"; + +/** + * Seed exactly as `renderWithAxis` does — player 0 == the local seat, with + * `active_player`/`turn_decision_controller` on that seat — so + * `useCanActForWaitingState` is true and no query below passes VACUOUSLY. + */ +function renderPrompt(waitingFor: WaitingFor) { + const seeded = setGameStoreForTest({ + gameState: buildGameState({ + waiting_for: waitingFor, + active_player: 0, + turn_decision_controller: 0, + }), + waitingFor, + }); + return { ...seeded, ...render() }; +} + +function type(value: string) { + fireEvent.change(screen.getByLabelText(BOX), { target: { value } }); +} + +describe("PayAmountChoiceUI — sanitized amount entry", () => { + beforeEach(() => { + useGameStore.getState().reset(); + }); + + afterEach(() => { + cleanup(); + }); + + it("T1/reach: a valid entry enables commit and dispatches exactly what was typed", () => { + const { dispatch } = renderPrompt(loopCollapseWaitingFor("Tokens")); + + // Positive control for every negative below: the harness reaches a rendered, + // enabled, dispatch-firing control. + expect(screen.getByLabelText(BOX)).toBeInTheDocument(); + type("377"); + + const commit = screen.getByRole("button", { name: /create 377 tokens/i }); + expect(commit).not.toBeDisabled(); + fireEvent.click(commit); + expect(dispatch).toHaveBeenCalledWith({ + type: "SubmitPayAmount", + data: { amount: 377 }, + }); + }); + + it("T2/above-max: an entry over the engine max cannot be committed", () => { + const { dispatch } = renderPrompt(loopCollapseWaitingFor("Tokens")); + type("1001"); + + expect(screen.getByLabelText(BOX)).toHaveValue("1001"); + expect(screen.getByRole("button", { name: /create/i })).toBeDisabled(); + expect(dispatch).not.toHaveBeenCalled(); + }); + + it("T3/below-min: an entry under the engine min cannot be committed", () => { + // Provenance honesty: `min > 0` is NOT currently reachable in production — every + // PayAmountChoice mint site hardcodes `min: 0`. This is a prop-contract test on the + // wire type, not a live-state repro. + const { dispatch } = renderPrompt( + buildPayAmountChoiceWaitingFor({ + data: { + player: 0, + resource: { type: "Counters" }, + min: 5, + max: 1000, + source_id: 0, + }, + }), + ); + type("3"); + + expect(screen.getByRole("button", { name: /^Pay /i })).toBeDisabled(); + expect( + screen.getByText("Enter a whole number between 5 and 1000"), + ).toBeInTheDocument(); + expect(dispatch).not.toHaveBeenCalled(); + }); + + // `min` is PINNED at 0 here: at min=5 the "+2" member becomes DOMINATED by the range + // conjunct and stops discriminating the digit guard. + it.each(["", " 7 ", "1.5", "1e3", "+2", "0x10"])( + "T4/non-numeric: %j never reaches the engine", + (raw) => { + const { dispatch } = renderPrompt(loopCollapseWaitingFor("Tokens")); + type(raw); + + expect(screen.getByRole("button", { name: /create/i })).toBeDisabled(); + expect(dispatch).not.toHaveBeenCalled(); + }, + ); + + it("T5/cleared: clearing a valid entry cannot submit the 0 the player never typed", () => { + const { dispatch } = renderPrompt(loopCollapseWaitingFor("Tokens")); + type("377"); + type(""); + + const commit = screen.getByRole("button", { name: /create/i }); + expect(commit).toBeDisabled(); + fireEvent.click(commit); + expect(dispatch).not.toHaveBeenCalled(); + }); + + it("T6a/enter-valid: Enter submits a valid entry", () => { + const { dispatch } = renderPrompt(loopCollapseWaitingFor("Tokens")); + type("42"); + fireEvent.keyDown(screen.getByLabelText(BOX), { key: "Enter" }); + + expect(dispatch).toHaveBeenCalledWith({ + type: "SubmitPayAmount", + data: { amount: 42 }, + }); + }); + + it("T6b/enter-invalid: Enter does NOT coerce an out-of-range entry", () => { + const { dispatch } = renderPrompt(loopCollapseWaitingFor("Tokens")); + type("1001"); + fireEvent.keyDown(screen.getByLabelText(BOX), { key: "Enter" }); + + expect(dispatch).not.toHaveBeenCalled(); + }); + + it("T7/engine-bound: the window comes from the engine, not a client constant", () => { + // Hostile multi-authority fixture: max=7, not the LoopCollapse 1000. Runnable proof + // that no client-side ceiling exists. + const { dispatch } = renderPrompt( + buildPayAmountChoiceWaitingFor({ + data: { + player: 0, + resource: { type: "LoopCollapse", data: { axis: "Tokens" } }, + min: 0, + max: 7, + source_id: 0, + }, + }), + ); + + type("8"); + expect(screen.getByRole("button", { name: /create/i })).toBeDisabled(); + + type("7"); + const commit = screen.getByRole("button", { name: /create 7 tokens/i }); + expect(commit).not.toBeDisabled(); + fireEvent.click(commit); + expect(dispatch).toHaveBeenCalledWith({ + type: "SubmitPayAmount", + data: { amount: 7 }, + }); + }); + + it("T8/seat-gate: the prompt renders only for the seat the engine addressed", () => { + // Rendering the component directly bypasses GamePage's outer gate, so the component's + // own `!data || !canAct` guard is genuinely the expression under test. + const { container } = renderPrompt( + buildPayAmountChoiceWaitingFor({ + data: { + player: 1, + resource: { type: "LoopCollapse", data: { axis: "Tokens" } }, + min: 0, + max: 1000, + source_id: 0, + }, + }), + ); + expect(container).toBeEmptyDOMElement(); + + // PINNED POSITIVE, same test: the emptiness above is satisfiable by a constant + // `return null`, so the identical prompt on the LOCAL seat must render the box. + cleanup(); + renderPrompt(loopCollapseWaitingFor("Tokens")); + expect(screen.getByLabelText(BOX)).toBeInTheDocument(); + }); + + it("T9/zero: 0 is a legal amount and commits (the truthiness-bug class)", () => { + const { dispatch } = renderPrompt(loopCollapseWaitingFor("Tokens")); + + // `raw` is "0" at mount (min = 0), so this asserts the mounted state directly. + expect(screen.getByLabelText(BOX)).toHaveValue("0"); + const commit = screen.getByRole("button", { name: /create 0 tokens/i }); + expect(commit).not.toBeDisabled(); + fireEvent.click(commit); + expect(dispatch).toHaveBeenCalledWith({ + type: "SubmitPayAmount", + data: { amount: 0 }, + }); + }); +}); diff --git a/client/src/i18n/locales/de/game.json b/client/src/i18n/locales/de/game.json index aab92ec833..91fcbed8bf 100644 --- a/client/src/i18n/locales/de/game.json +++ b/client/src/i18n/locales/de/game.json @@ -577,6 +577,9 @@ "xEquals": "X = {{value}}", "minMax": "min {{min}} / max {{max}}", "maxOnly": "max {{max}}", + "amountOutOfRange": "Gib eine ganze Zahl zwischen {{min}} und {{max}} ein", + "decreaseAmount": "Menge verringern", + "increaseAmount": "Menge erhöhen", "confirmX": "X = {{value}} bestätigen", "payMana": "Manakosten bezahlen", "convokeHint": "Tappe Kreaturen, um die Bezahlung zu unterstützen.", diff --git a/client/src/i18n/locales/en/game.json b/client/src/i18n/locales/en/game.json index c89f3f9f02..badbfb6d9c 100644 --- a/client/src/i18n/locales/en/game.json +++ b/client/src/i18n/locales/en/game.json @@ -616,6 +616,9 @@ "xEquals": "X = {{value}}", "minMax": "min {{min}} / max {{max}}", "maxOnly": "max {{max}}", + "amountOutOfRange": "Enter a whole number between {{min}} and {{max}}", + "decreaseAmount": "Decrease amount", + "increaseAmount": "Increase amount", "confirmX": "Confirm X = {{value}}", "payMana": "Pay Mana Cost", "convokeHint": "Tap creatures to help pay.", diff --git a/client/src/i18n/locales/es/game.json b/client/src/i18n/locales/es/game.json index 15eec7374d..4b200251fb 100644 --- a/client/src/i18n/locales/es/game.json +++ b/client/src/i18n/locales/es/game.json @@ -577,6 +577,9 @@ "xEquals": "X = {{value}}", "minMax": "mín. {{min}} / máx. {{max}}", "maxOnly": "máx. {{max}}", + "amountOutOfRange": "Introduce un número entero entre {{min}} y {{max}}", + "decreaseAmount": "Disminuir la cantidad", + "increaseAmount": "Aumentar la cantidad", "confirmX": "Confirmar X = {{value}}", "payMana": "Pagar el coste de maná", "convokeHint": "Gira criaturas para ayudar a pagar.", diff --git a/client/src/i18n/locales/fr/game.json b/client/src/i18n/locales/fr/game.json index 36f73a2fd0..339e6c5230 100644 --- a/client/src/i18n/locales/fr/game.json +++ b/client/src/i18n/locales/fr/game.json @@ -577,6 +577,9 @@ "xEquals": "X = {{value}}", "minMax": "min {{min}} / max {{max}}", "maxOnly": "max {{max}}", + "amountOutOfRange": "Entrez un nombre entier entre {{min}} et {{max}}", + "decreaseAmount": "Diminuer la quantité", + "increaseAmount": "Augmenter la quantité", "confirmX": "Confirmer X = {{value}}", "payMana": "Payer le coût de mana", "convokeHint": "Engagez des créatures pour aider à payer.", diff --git a/client/src/i18n/locales/it/game.json b/client/src/i18n/locales/it/game.json index 1024e7baec..e503d310a9 100644 --- a/client/src/i18n/locales/it/game.json +++ b/client/src/i18n/locales/it/game.json @@ -577,6 +577,9 @@ "xEquals": "X = {{value}}", "minMax": "min {{min}} / max {{max}}", "maxOnly": "max {{max}}", + "amountOutOfRange": "Inserisci un numero intero tra {{min}} e {{max}}", + "decreaseAmount": "Diminuisci la quantità", + "increaseAmount": "Aumenta la quantità", "confirmX": "Conferma X = {{value}}", "payMana": "Paga il costo di mana", "convokeHint": "TAPpa le creature per aiutare a pagare.", diff --git a/client/src/i18n/locales/pl/game.json b/client/src/i18n/locales/pl/game.json index c7e3139e9b..b7060edaca 100644 --- a/client/src/i18n/locales/pl/game.json +++ b/client/src/i18n/locales/pl/game.json @@ -577,6 +577,9 @@ "xEquals": "X = {{value}}", "minMax": "min {{min}} / maks {{max}}", "maxOnly": "maks {{max}}", + "amountOutOfRange": "Wpisz liczbę całkowitą od {{min}} do {{max}}", + "decreaseAmount": "Zmniejsz kwotę", + "increaseAmount": "Zwiększ kwotę", "confirmX": "Potwierdź X = {{value}}", "payMana": "Zapłać koszt many", "convokeHint": "Obróć stwory, aby pomóc zapłacić.", diff --git a/client/src/i18n/locales/pt/game.json b/client/src/i18n/locales/pt/game.json index 3dfe39ebaf..daf79b52fd 100644 --- a/client/src/i18n/locales/pt/game.json +++ b/client/src/i18n/locales/pt/game.json @@ -577,6 +577,9 @@ "xEquals": "X = {{value}}", "minMax": "mín {{min}} / máx {{max}}", "maxOnly": "máx {{max}}", + "amountOutOfRange": "Digite um número inteiro entre {{min}} e {{max}}", + "decreaseAmount": "Diminuir a quantia", + "increaseAmount": "Aumentar a quantia", "confirmX": "Confirmar X = {{value}}", "payMana": "Pagar Custo de Mana", "convokeHint": "Vire criaturas para ajudar a pagar.", diff --git a/client/src/test/factories/gameStateFactory.ts b/client/src/test/factories/gameStateFactory.ts index 88fec89e54..4d4d0f9164 100644 --- a/client/src/test/factories/gameStateFactory.ts +++ b/client/src/test/factories/gameStateFactory.ts @@ -29,6 +29,7 @@ type TriggerTargetSelectionWaitingFor = Extract< { type: "TriggerTargetSelection" } >; type ChooseXValueWaitingFor = Extract; +type PayAmountChoiceWaitingFor = Extract; type UntapChoiceWaitingFor = Extract; type AssistPaymentWaitingFor = Extract; type CastOfferWaitingFor = Extract; @@ -284,6 +285,26 @@ export const buildChooseXValueWaitingFor = ( return chooseXValueWaitingForFactory.withData(overrides.data ?? {}).build(); }; +export class PayAmountChoiceWaitingForFactory extends PlayerWaitingForFactory {} + +export const payAmountChoiceWaitingForFactory = + PayAmountChoiceWaitingForFactory.define((): PayAmountChoiceWaitingFor => ({ + type: "PayAmountChoice", + data: { + player: 0, + resource: { type: "Energy" }, + min: 0, + max: 0, + source_id: 0, + }, + })); + +export const buildPayAmountChoiceWaitingFor = ( + overrides: Partial = {}, +): PayAmountChoiceWaitingFor => { + return payAmountChoiceWaitingForFactory.withData(overrides.data ?? {}).build(); +}; + export class AssistPaymentWaitingForFactory extends WaitingForFactory { withCaster(caster: PlayerId) { return this.withData({ caster }); @@ -392,6 +413,10 @@ export class WaitingForVariantFactory extends Factory = {}) { + return this.variant(payAmountChoiceWaitingForFactory.withData(data).build()); + } + assistPayment(data: Partial = {}) { return this.variant(assistPaymentWaitingForFactory.withData(data).build()); } @@ -635,6 +660,10 @@ export class GameStateFactory extends Factory { return this.waitingFor(waitingForFactory.chooseXValue(data).build()); } + payAmountChoice(data: Partial = {}) { + return this.waitingFor(waitingForFactory.payAmountChoice(data).build()); + } + assistPayment(data: Partial = {}) { return this.waitingFor(waitingForFactory.assistPayment(data).build()); } From 5dbc9cc874ff9ac8729145df91ac576c5090a472 Mon Sep 17 00:00:00 2001 From: lgray Date: Tue, 4 Aug 2026 22:41:59 -0500 Subject: [PATCH 2/9] feat(client): adopt the shared amount box in the ChooseX and Assist prompts MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Both remaining amount prompts drove their own ``, and ChooseXValue additionally carried a hand-rolled number box with its own clamping. Three prompts, three sanitization stories, one of which silently rewrote the player's input. All three now share `AmountInput`, so the [min, max] window, the digit-only gate, the keyboard contract and the validation copy have one authority. DELIBERATE BEHAVIOUR CHANGE in the ChooseXValue cast prompt. Out-of-range X is now REJECTED rather than coerced: typing 99 under max=5 used to commit 5, and typing 0 under min=2 used to commit 2 — values the caster never chose, which CR 601.2f makes the caster's decision. Both now disable Confirm and show the range message. A third shipped assertion flips with them: the steppers no longer disable while the entry is out of range, because with the slider gone they are the recovery path back into the window. The two affected tests are rewritten in place and marked DELIBERATE rather than silently edited. The #2427 DialogHost guard is RETARGETED, not deleted. Its subject was the range control, but it never asserted hit-testing — `fireEvent.change` dispatches on the node and happy-dom performs no layout — so it was always a mount-integration guard. It now drives the box and a stepper, which are the surfaces a #2427-class regression would break next. Each prompt gains an Enter test. Enter bypasses the commit button's `disabled` attribute, so it is the only route on which `handleCommit`'s null-guard is observable at all; without these the guard was covered only by the attribute and a coercion regression would have shipped green. Both are matched pairs, so a component that simply never dispatches cannot satisfy them. `mana.chooseXAria` and `mana.xEquals` are deleted across all 7 locales: the slider carried the first and both readouts carried the second, and a bare substring census finds no remaining use, dynamic key construction included. CR 702.132a verified against docs/MagicCompRules.txt: "the player you chose may pay for any amount of the generic mana in the spell's total cost" — the assist domain is 0..max_generic, which is what the engine's own `number_projection` synthesizes as `min: 0`. The client mirrors the engine; it invents no bound. Assisted-by: ClaudeCode:claude-opus-5 --- .../src/components/mana/AssistPaymentUI.tsx | 59 +++++---- client/src/components/mana/ChooseXValueUI.tsx | 113 +++++------------- .../mana/__tests__/AssistPaymentUI.test.tsx | 71 +++++++++-- .../mana/__tests__/ChooseXValueUI.test.tsx | 92 +++++++++++--- client/src/i18n/locales/de/game.json | 2 - client/src/i18n/locales/en/game.json | 2 - client/src/i18n/locales/es/game.json | 2 - client/src/i18n/locales/fr/game.json | 2 - client/src/i18n/locales/it/game.json | 2 - client/src/i18n/locales/pl/game.json | 2 - client/src/i18n/locales/pt/game.json | 2 - 11 files changed, 202 insertions(+), 147 deletions(-) diff --git a/client/src/components/mana/AssistPaymentUI.tsx b/client/src/components/mana/AssistPaymentUI.tsx index 2224db42c4..e24bdd191f 100644 --- a/client/src/components/mana/AssistPaymentUI.tsx +++ b/client/src/components/mana/AssistPaymentUI.tsx @@ -5,6 +5,7 @@ import { useTranslation } from "react-i18next"; import { useCanActForWaitingState } from "../../hooks/usePlayerId.ts"; import { useGameStore } from "../../stores/gameStore.ts"; import { gameButtonClass } from "../ui/buttonStyles.ts"; +import { AmountInput, parseAmount } from "./AmountInput.tsx"; /** * CR 702.132a: Assist — the chosen helper decides how much of the spell's @@ -21,15 +22,23 @@ export function AssistPaymentUI() { const isAssistPayment = waitingFor?.type === "AssistPayment"; const max = isAssistPayment ? waitingFor.data.max_generic : 0; - const [value, setValue] = useState(0); + const [raw, setRaw] = useState("0"); useEffect(() => { - if (isAssistPayment) setValue(0); + if (isAssistPayment) setRaw("0"); }, [isAssistPayment, max]); + // CR 702.132a: "the player you chose may pay for any amount of the generic mana in the + // spell's total cost" — the variant's domain is 0..max_generic. This is NOT a + // frontend-invented bound: `WaitingFor::AssistPayment` carries no `min` field, and the + // engine's own prompt projection (`game::interaction::number_projection`) synthesizes + // `min: 0` for this variant. The client mirrors the engine's projection. + const amount = parseAmount(raw, 0, max); + const handleCommit = useCallback(() => { - dispatch({ type: "CommitAssistPayment", data: { generic: value } }); - }, [dispatch, value]); + if (amount === null) return; + dispatch({ type: "CommitAssistPayment", data: { generic: amount } }); + }, [dispatch, amount]); if (!isAssistPayment || !canAct) return null; @@ -47,32 +56,34 @@ export function AssistPaymentUI() { {t("assist.payment.title")} -
- -
+ {/* Reusing `assist.payment.title` as the box's accessible name preserves today's + accessible name byte-for-byte (the slider already carried it) and adds no key. */} +
diff --git a/client/src/components/mana/ChooseXValueUI.tsx b/client/src/components/mana/ChooseXValueUI.tsx index e3803f219d..969ede8c30 100644 --- a/client/src/components/mana/ChooseXValueUI.tsx +++ b/client/src/components/mana/ChooseXValueUI.tsx @@ -6,6 +6,7 @@ import { useCanActForWaitingState } from "../../hooks/usePlayerId.ts"; import { useGameStore } from "../../stores/gameStore.ts"; import { manaCostToShards } from "../../viewmodel/costLabel.ts"; import { gameButtonClass } from "../ui/buttonStyles.ts"; +import { AmountInput, parseAmount } from "./AmountInput.tsx"; import { ManaSymbol } from "./ManaSymbol.tsx"; /** @@ -31,16 +32,16 @@ export function ChooseXValueUI() { const pendingCast = isChooseX ? waitingFor.data.pending_cast : null; const xCostPreviews = isChooseX ? waitingFor.data.x_cost_previews : undefined; - const [value, setValue] = useState(0); - const clampedValue = Math.min(Math.max(value, min), max); + const [raw, setRaw] = useState(String(defaultValue)); + const amount = parseAmount(raw, min, max); const pendingCostShards = useMemo(() => { if (!pendingCast) return null; - const previewCost = xCostPreviews?.find(([x]) => x === clampedValue)?.[1]; + const previewCost = xCostPreviews?.find(([x]) => x === (amount ?? min))?.[1]; const cost = previewCost ?? pendingCast.cost; const shards = manaCostToShards(cost); return shards.length > 0 ? shards : null; - }, [pendingCast, xCostPreviews, clampedValue]); + }, [pendingCast, xCostPreviews, amount, min]); const cardName = useMemo(() => { if (!gameState || !pendingCast) return null; @@ -48,28 +49,18 @@ export function ChooseXValueUI() { }, [gameState, pendingCast]); useEffect(() => { - if (isChooseX) setValue(defaultValue); + if (isChooseX) setRaw(String(defaultValue)); }, [isChooseX, defaultValue]); - const clampValue = useCallback( - (nextValue: number) => Math.min(Math.max(nextValue, min), max), - [max, min], - ); - - const handleValueChange = useCallback( - (nextValue: number) => { - if (!Number.isFinite(nextValue)) return; - setValue(Math.min(nextValue, max)); - }, - [max], - ); - const handleCommit = useCallback(() => { + // Sanitization gate: an out-of-range X is REJECTED, not clamped. Typing 99 under max=5 + // used to silently cast for 5 — a value the caster never chose (CR 601.2f). + if (amount === null) return; dispatch({ type: "ChooseX", - data: { value: clampedValue }, + data: { value: amount }, }); - }, [clampedValue, dispatch]); + }, [amount, dispatch]); const handleCancel = useCallback(() => { dispatch({ type: "CancelCast" }); @@ -107,74 +98,30 @@ export function ChooseXValueUI() { )} -
- -
- - handleValueChange(Number(e.target.value))} - onBlur={() => setValue(clampedValue)} - aria-label={t("mana.chooseXInputAria")} - className="h-9 w-20 rounded-lg border border-cyan-400/30 bg-gray-950/80 px-2 text-center font-mono text-base font-semibold text-cyan-100 shadow-inner outline-none transition focus:border-cyan-300 focus:ring-2 focus:ring-cyan-400/30" - /> - -
-
+
diff --git a/client/src/components/mana/ChooseXValueUI.tsx b/client/src/components/mana/ChooseXValueUI.tsx index 969ede8c30..d318b9cfab 100644 --- a/client/src/components/mana/ChooseXValueUI.tsx +++ b/client/src/components/mana/ChooseXValueUI.tsx @@ -12,8 +12,10 @@ import { ManaSymbol } from "./ManaSymbol.tsx"; /** * Overlay for the `WaitingFor::ChooseXValue` state. * - * CR 107.1b + CR 601.2f: X must be chosen as part of determining total cost, - * before mana is paid. The engine computes the upper bound (`max`) from the + * CR 107.3a + CR 601.2b: the controller of the spell chooses and announces X as part of + * casting it; CR 601.2f then locks in the total cost that X feeds, so the value must be + * settled before mana is paid. CR 107.1b (a negative number can't be chosen) is why the + * lower bound is never below 0. The engine computes the upper bound (`max`) from the * player's pool + untapped free-to-tap producers; this component is a pure * display layer that dispatches the caster's chosen value via `ChooseX`. */ @@ -37,11 +39,15 @@ export function ChooseXValueUI() { const pendingCostShards = useMemo(() => { if (!pendingCast) return null; - const previewCost = xCostPreviews?.find(([x]) => x === (amount ?? min))?.[1]; + // While the entry is invalid there is no chosen X, so there is no cost to preview. Falling + // back to `min` here would render the mana cost of an X the caster never typed — the same + // defect on the display side that the commit guard below fixes on the dispatch side. + if (amount === null) return null; + const previewCost = xCostPreviews?.find(([x]) => x === amount)?.[1]; const cost = previewCost ?? pendingCast.cost; const shards = manaCostToShards(cost); return shards.length > 0 ? shards : null; - }, [pendingCast, xCostPreviews, amount, min]); + }, [pendingCast, xCostPreviews, amount]); const cardName = useMemo(() => { if (!gameState || !pendingCast) return null; @@ -50,11 +56,17 @@ export function ChooseXValueUI() { useEffect(() => { if (isChooseX) setRaw(String(defaultValue)); - }, [isChooseX, defaultValue]); + // `max` is a dependency even though it does not appear in the body: re-entering ChooseXValue + // with a NARROWER max but an unchanged min leaves `defaultValue` identical, so without it the + // effect would not fire and a now-out-of-range entry would persist with no way to self-heal. + // Both sibling prompts already key their reset on the full window (PayAmountChoiceUI on + // [min, max], AssistPaymentUI on [max]); this closes that asymmetry. + }, [isChooseX, defaultValue, max]); const handleCommit = useCallback(() => { // Sanitization gate: an out-of-range X is REJECTED, not clamped. Typing 99 under max=5 - // used to silently cast for 5 — a value the caster never chose (CR 601.2f). + // used to silently cast for 5 — a value the caster never chose. CR 107.3a: the controller + // "chooses and announces the value of X"; CR 601.2f governs total cost, not that choice. if (amount === null) return; dispatch({ type: "ChooseX", @@ -66,8 +78,8 @@ export function ChooseXValueUI() { dispatch({ type: "CancelCast" }); }, [dispatch]); - // CR 601.2f: X is chosen by the caster; opponents observe via the stack - // ghost entry, not an interactive panel. + // CR 107.3a: X is chosen and announced by the spell's controller, so only that player gets + // this panel; opponents observe via the stack ghost entry, not an interactive panel. if (!isChooseX || !canAct || !hasValidBounds) return null; return ( @@ -121,7 +133,11 @@ export function ChooseXValueUI() { disabled: amount === null, })} > - {t("mana.confirmX", { value: amount ?? min })} + {/* No chosen X yet ⇒ name the action without a value. `amount ?? min` here would + label the button "Confirm X = " while the player has 99 typed. */} + {amount === null + ? t("mana.confirmAmount") + : t("mana.confirmX", { value: amount })} diff --git a/client/src/components/mana/__tests__/AmountInput.test.tsx b/client/src/components/mana/__tests__/AmountInput.test.tsx index 50fadde731..efd4c61d92 100644 --- a/client/src/components/mana/__tests__/AmountInput.test.tsx +++ b/client/src/components/mana/__tests__/AmountInput.test.tsx @@ -121,6 +121,87 @@ describe("AmountInput — rendered contract", () => { expect(document.getElementById(validIds[0])?.textContent).toBe("max 5"); }); + // A11Y/value: the three controls this box replaced announced their value natively + // (`type="range"` ⇒ slider, `type="number"` ⇒ spinbutton). A bare `type="text"` announces + // nothing, so stepping with +/− or the arrow keys would mutate a value exposed by NOTHING — + // the live region below carries only the refusal, never the accepted amount. + it("A11Y/value: the accepted amount is exposed, and withheld while out of range", () => { + const { rerender } = render( + , + ); + + const box = screen.getByLabelText("Enter amount"); + expect(box).toHaveAttribute("role", "spinbutton"); + expect(box).toHaveAttribute("aria-valuenow", "3"); + expect(box).toHaveAttribute("aria-valuemin", "0"); + expect(box).toHaveAttribute("aria-valuemax", "5"); + + // Out of range: the value is WITHHELD rather than reported, so a screen reader is never told + // a valuenow that contradicts valuemax. `aria-invalid` carries that state instead. + rerender( + , + ); + const invalidBox = screen.getByLabelText("Enter amount"); + expect(invalidBox).not.toHaveAttribute("aria-valuenow"); + expect(invalidBox).toHaveAttribute("aria-invalid", "true"); + }); + + // A11Y/live-region: the refusal must be ANNOUNCED, not merely associated. `aria-describedby` + // is resolved by a screen reader when focus ARRIVES at the box, but the message appears while + // focus is already inside it — so association alone is silent, and the player would have to tab + // away and back to learn the entry was refused. The region must therefore PRE-EXIST its message + // and only mutate its text; inserting the region (or inserting a node into one) is not + // reliably announced. + it("A11Y/live-region: the validation copy lives in a polite region that pre-exists it", () => { + const { rerender } = render( + , + ); + + // VALID first. This is the half that reds under a conditional mount, where the node does not + // exist until the entry goes invalid. + const region = document.querySelector('[role="status"]'); + expect(region).not.toBeNull(); + expect(region).toHaveAttribute("aria-live", "polite"); + expect(region).toBeEmptyDOMElement(); + + rerender( + , + ); + + // The SAME node, with mutated text — that identity is the property that makes it audible. + // A different node here would mean the message was inserted rather than announced. + expect(document.querySelector('[role="status"]')).toBe(region); + expect(region).toHaveTextContent("Enter a whole number between 0 and 5"); + }); + it("A11Y/arrows: ArrowUp/ArrowDown step and clamp to the window", () => { const onRawChange = vi.fn(); const { rerender } = render( diff --git a/client/src/components/mana/__tests__/AssistPaymentUI.test.tsx b/client/src/components/mana/__tests__/AssistPaymentUI.test.tsx index 0f624195e5..c5404430fa 100644 --- a/client/src/components/mana/__tests__/AssistPaymentUI.test.tsx +++ b/client/src/components/mana/__tests__/AssistPaymentUI.test.tsx @@ -156,7 +156,14 @@ describe("AssistPaymentUI", () => { target: { value: "5" }, }); - expect(screen.getByRole("button", { name: "Pay nothing" })).toBeDisabled(); + // "Pay nothing" here was the OPPOSITE of the pending intent: the player typed 5, and + // `(amount ?? 0) === 0` collapsed the invalid entry into the decline label. Declining is + // amount === 0 exactly (pinned separately above); an invalid entry names no amount at all. + expect(screen.getByRole("button", { name: "Confirm" })).toBeDisabled(); + // DOMINATED by the assertion above, labelled rather than deleted. MEASURED on the sibling + // prompt: removing the null-guard reds only the Enter row, and a click here cannot help + // because React does not dispatch `onClick` to a disabled `button` (a React-level + // suppression, not a property of the test environment). AP/enter discriminates. expect(dispatch).not.toHaveBeenCalled(); }); }); diff --git a/client/src/components/mana/__tests__/ChooseXValueUI.test.tsx b/client/src/components/mana/__tests__/ChooseXValueUI.test.tsx index e0da735404..257fb43d45 100644 --- a/client/src/components/mana/__tests__/ChooseXValueUI.test.tsx +++ b/client/src/components/mana/__tests__/ChooseXValueUI.test.tsx @@ -142,14 +142,16 @@ describe("ChooseXValueUI", () => { const input = screen.getByLabelText("Enter X value") as HTMLInputElement; fireEvent.change(input, { target: { value: "99" } }); expect(input.value).toBe("99"); - expect(screen.getByRole("button", { name: /^Confirm X/ })).toBeDisabled(); + // The label carries NO value while the entry is invalid. `/^Confirm X/` would also match + // "Confirm X = 2" — an X the caster never typed — so the neutral verb is asserted exactly. + expect(screen.getByRole("button", { name: "Confirm" })).toBeDisabled(); expect( screen.getByText("Enter a whole number between 2 and 5"), ).toBeInTheDocument(); fireEvent.change(input, { target: { value: "0" } }); expect(input.value).toBe("0"); - expect(screen.getByRole("button", { name: /^Confirm X/ })).toBeDisabled(); + expect(screen.getByRole("button", { name: "Confirm" })).toBeDisabled(); expect( screen.getByText("Enter a whole number between 2 and 5"), ).toBeInTheDocument(); @@ -163,6 +165,30 @@ describe("ChooseXValueUI", () => { expect(input.value).toBe("0"); }); + // The cost preview displays the CHOSEN X. While the entry is invalid there is no chosen X, so + // the previous `amount ?? min` fallback rendered the mana cost of a value the caster never + // typed — the display-side twin of the commit guard. + it("hides the pending cost preview while the entry is invalid", () => { + const waitingFor = chooseXWaitingFor(5, 2); + + setGameStoreForTest({ + gameState: createGameState({ waiting_for: waitingFor }), + waitingFor, + }); + + render(); + + // Paired positive: without this half, a component that never rendered pips at all would + // satisfy the negative below. + expect(screen.getAllByAltText("G").length).toBeGreaterThan(0); + + fireEvent.change(screen.getByLabelText("Enter X value"), { + target: { value: "99" }, + }); + + expect(screen.queryByAltText("G")).not.toBeInTheDocument(); + }); + // DELIBERATE BEHAVIOUR CHANGE (binding ruling D2): the intermediate "1" no longer reads as // a committed 10. The multi-digit-typing capability it guards SURVIVES — the final dispatch // assertion is unchanged. @@ -181,7 +207,7 @@ describe("ChooseXValueUI", () => { const input = screen.getByLabelText("Enter X value") as HTMLInputElement; fireEvent.change(input, { target: { value: "1" } }); expect(input.value).toBe("1"); - expect(screen.getByRole("button", { name: /^Confirm X/ })).toBeDisabled(); + expect(screen.getByRole("button", { name: "Confirm" })).toBeDisabled(); fireEvent.change(input, { target: { value: "15" } }); expect(input.value).toBe("15"); @@ -263,6 +289,47 @@ describe("ChooseXValueUI", () => { expect(screen.getByRole("button", { name: "Confirm X = 2" })).toBeInTheDocument(); }); + // The test ABOVE cannot discriminate the reset's `max` dependency: it narrows min 1 → 2 as + // well, so `defaultValue` changes and the effect fires either way. This row holds min CONSTANT, + // which is the only shape in which a reset keyed on `defaultValue` alone is observably wrong. + // Both sibling prompts already key on the full window (PayAmountChoiceUI [min, max], + // AssistPaymentUI [max]) — this closes that asymmetry. + it("resets an entry stranded above a narrowed max when min is unchanged", () => { + const dispatch = vi.fn().mockResolvedValue([]); + const waitingFor = chooseXWaitingFor(10, 0); + + setGameStoreForTest({ + gameState: createGameState({ waiting_for: waitingFor }), + waitingFor, + dispatch, + }); + + const { rerender } = render(); + + fireEvent.change(screen.getByLabelText("Enter X value"), { + target: { value: "7" }, + }); + expect( + screen.getByRole("button", { name: "Confirm X = 7" }), + ).toBeInTheDocument(); + + // Same min (0), narrower max (10 → 4). Without `max` in the dep array the entry stays "7", + // which is now permanently refused with no reset to recover it. + const nextWaitingFor = chooseXWaitingFor(4, 0); + setGameStoreForTest({ + gameState: createGameState({ waiting_for: nextWaitingFor }), + waitingFor: nextWaitingFor, + dispatch, + }); + + rerender(); + + expect(screen.getByLabelText("Enter X value")).toHaveValue("0"); + expect( + screen.getByRole("button", { name: "Confirm X = 0" }), + ).toBeInTheDocument(); + }); + it("renders nothing for impossible min greater than max bounds", () => { const waitingFor = chooseXWaitingFor(0, 1); diff --git a/client/src/components/mana/__tests__/PayAmountChoiceUI.test.tsx b/client/src/components/mana/__tests__/PayAmountChoiceUI.test.tsx index 8dccca2d06..fd66a26aef 100644 --- a/client/src/components/mana/__tests__/PayAmountChoiceUI.test.tsx +++ b/client/src/components/mana/__tests__/PayAmountChoiceUI.test.tsx @@ -144,7 +144,16 @@ describe("PayAmountChoiceUI — sanitized amount entry", () => { type("1001"); expect(screen.getByLabelText(BOX)).toHaveValue("1001"); - expect(screen.getByRole("button", { name: /create/i })).toBeDisabled(); + // Name pinned to the neutral verb: "Create 0 tokens" here would state an amount the player + // never typed, and `/create/i` would match it. + expect(screen.getByRole("button", { name: "Confirm" })).toBeDisabled(); + // DOMINATED by the assertion above — labelled, per this file's convention, so it is not + // mistaken for a probe of `handleCommit`'s null-guard. MEASURED: deleting + // `if (amount === null) return;` reds ONLY T6b, and adding a click here does not change + // that, because React does not dispatch `onClick` to a disabled `button` at all. That + // suppression is React-level, not environment-level (this suite runs happy-dom), so + // switching the test environment would not change the calculus. Enter (T6b) bypasses + // `disabled` and is the only route on which that guard is observable. expect(dispatch).not.toHaveBeenCalled(); }); @@ -165,10 +174,11 @@ describe("PayAmountChoiceUI — sanitized amount entry", () => { ); type("3"); - expect(screen.getByRole("button", { name: /^Pay /i })).toBeDisabled(); + expect(screen.getByRole("button", { name: "Confirm" })).toBeDisabled(); expect( screen.getByText("Enter a whole number between 5 and 1000"), ).toBeInTheDocument(); + // DOMINATED — see T2. The discriminating row for the null-guard is T6b. expect(dispatch).not.toHaveBeenCalled(); }); @@ -180,7 +190,8 @@ describe("PayAmountChoiceUI — sanitized amount entry", () => { const { dispatch } = renderPrompt(loopCollapseWaitingFor("Tokens")); type(raw); - expect(screen.getByRole("button", { name: /create/i })).toBeDisabled(); + expect(screen.getByRole("button", { name: "Confirm" })).toBeDisabled(); + // DOMINATED — see T2. The discriminating row for the null-guard is T6b. expect(dispatch).not.toHaveBeenCalled(); }, ); @@ -190,9 +201,10 @@ describe("PayAmountChoiceUI — sanitized amount entry", () => { type("377"); type(""); - const commit = screen.getByRole("button", { name: /create/i }); - expect(commit).toBeDisabled(); - fireEvent.click(commit); + expect(screen.getByRole("button", { name: "Confirm" })).toBeDisabled(); + // The click that used to sit here was inert — React does not dispatch `onClick` to a + // disabled `button` — and implied coverage this row does not have. DOMINATED; T6b + // discriminates. See T2. expect(dispatch).not.toHaveBeenCalled(); }); @@ -231,7 +243,7 @@ describe("PayAmountChoiceUI — sanitized amount entry", () => { ); type("8"); - expect(screen.getByRole("button", { name: /create/i })).toBeDisabled(); + expect(screen.getByRole("button", { name: "Confirm" })).toBeDisabled(); type("7"); const commit = screen.getByRole("button", { name: /create 7 tokens/i }); diff --git a/client/src/components/modal/DialogHost.tsx b/client/src/components/modal/DialogHost.tsx index adeb15b012..cf3664470e 100644 --- a/client/src/components/modal/DialogHost.tsx +++ b/client/src/components/modal/DialogHost.tsx @@ -176,9 +176,13 @@ export function DialogHost({ children }: { children: ReactNode }) { const isNarrow = useIsNarrowViewport(); // Only apply the peek slide transform while peeked. Framer-motion keeps a // residual `transform` (even at `{ x: 0, y: 0 }`) whenever `animate` is set, - // which breaks `` hit-testing in bottom-anchored panels - // such as ChooseXValueUI — the slider looks fine but ignores drags until - // something else reflows the tree (issue #2427). + // which breaks pointer hit-testing in bottom-anchored panels — the control + // looks fine but ignores input until something else reflows the tree + // (issue #2427). Originally hit `` in ChooseXValueUI; + // that slider no longer exists, and the live subjects are now the amount box + // and its ± steppers (AmountInput), which ChooseXValueUI's mount-integration + // test drives for exactly this reason. The guard is NOT obsolete just because + // the control it was written for is gone. const slideTransform = peeked ? isNarrow ? { x: 0, y: "calc(100vh - 64px)" } diff --git a/client/src/i18n/locales/de/game.json b/client/src/i18n/locales/de/game.json index 0d16a4efe5..ba9858a5d3 100644 --- a/client/src/i18n/locales/de/game.json +++ b/client/src/i18n/locales/de/game.json @@ -578,6 +578,7 @@ "amountOutOfRange": "Gib eine ganze Zahl zwischen {{min}} und {{max}} ein", "decreaseAmount": "Menge verringern", "increaseAmount": "Menge erhöhen", + "confirmAmount": "Bestätigen", "confirmX": "X = {{value}} bestätigen", "payMana": "Manakosten bezahlen", "convokeHint": "Tappe Kreaturen, um die Bezahlung zu unterstützen.", diff --git a/client/src/i18n/locales/en/game.json b/client/src/i18n/locales/en/game.json index 153af2013f..0ecc33951c 100644 --- a/client/src/i18n/locales/en/game.json +++ b/client/src/i18n/locales/en/game.json @@ -617,6 +617,7 @@ "amountOutOfRange": "Enter a whole number between {{min}} and {{max}}", "decreaseAmount": "Decrease amount", "increaseAmount": "Increase amount", + "confirmAmount": "Confirm", "confirmX": "Confirm X = {{value}}", "payMana": "Pay Mana Cost", "convokeHint": "Tap creatures to help pay.", diff --git a/client/src/i18n/locales/es/game.json b/client/src/i18n/locales/es/game.json index 395406e1d8..471c4aace7 100644 --- a/client/src/i18n/locales/es/game.json +++ b/client/src/i18n/locales/es/game.json @@ -578,6 +578,7 @@ "amountOutOfRange": "Introduce un número entero entre {{min}} y {{max}}", "decreaseAmount": "Disminuir la cantidad", "increaseAmount": "Aumentar la cantidad", + "confirmAmount": "Confirmar", "confirmX": "Confirmar X = {{value}}", "payMana": "Pagar el coste de maná", "convokeHint": "Gira criaturas para ayudar a pagar.", diff --git a/client/src/i18n/locales/fr/game.json b/client/src/i18n/locales/fr/game.json index 12e37eb41f..6054e17c24 100644 --- a/client/src/i18n/locales/fr/game.json +++ b/client/src/i18n/locales/fr/game.json @@ -578,6 +578,7 @@ "amountOutOfRange": "Entrez un nombre entier entre {{min}} et {{max}}", "decreaseAmount": "Diminuer la quantité", "increaseAmount": "Augmenter la quantité", + "confirmAmount": "Confirmer", "confirmX": "Confirmer X = {{value}}", "payMana": "Payer le coût de mana", "convokeHint": "Engagez des créatures pour aider à payer.", diff --git a/client/src/i18n/locales/it/game.json b/client/src/i18n/locales/it/game.json index 0aa8156d2a..54435a0cc3 100644 --- a/client/src/i18n/locales/it/game.json +++ b/client/src/i18n/locales/it/game.json @@ -578,6 +578,7 @@ "amountOutOfRange": "Inserisci un numero intero tra {{min}} e {{max}}", "decreaseAmount": "Diminuisci la quantità", "increaseAmount": "Aumenta la quantità", + "confirmAmount": "Conferma", "confirmX": "Conferma X = {{value}}", "payMana": "Paga il costo di mana", "convokeHint": "TAPpa le creature per aiutare a pagare.", diff --git a/client/src/i18n/locales/pl/game.json b/client/src/i18n/locales/pl/game.json index 78b60d8c04..adbabae652 100644 --- a/client/src/i18n/locales/pl/game.json +++ b/client/src/i18n/locales/pl/game.json @@ -578,6 +578,7 @@ "amountOutOfRange": "Wpisz liczbę całkowitą od {{min}} do {{max}}", "decreaseAmount": "Zmniejsz kwotę", "increaseAmount": "Zwiększ kwotę", + "confirmAmount": "Potwierdź", "confirmX": "Potwierdź X = {{value}}", "payMana": "Zapłać koszt many", "convokeHint": "Obróć stwory, aby pomóc zapłacić.", diff --git a/client/src/i18n/locales/pt/game.json b/client/src/i18n/locales/pt/game.json index d977040b4f..10f18eb93e 100644 --- a/client/src/i18n/locales/pt/game.json +++ b/client/src/i18n/locales/pt/game.json @@ -578,6 +578,7 @@ "amountOutOfRange": "Digite um número inteiro entre {{min}} e {{max}}", "decreaseAmount": "Diminuir a quantia", "increaseAmount": "Aumentar a quantia", + "confirmAmount": "Confirmar", "confirmX": "Confirmar X = {{value}}", "payMana": "Pagar Custo de Mana", "convokeHint": "Vire criaturas para ajudar a pagar.", From 35effbdc89fe13efbb06808a750d80085eee5fdc Mon Sep 17 00:00:00 2001 From: lgray Date: Wed, 5 Aug 2026 00:29:38 -0500 Subject: [PATCH 4/9] fix(client): key amount-prompt resets on prompt identity, and widen the stepper touch target MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Two CodeRabbit findings on #7019, both valid. The three reset effects observed only prompt type and bounds, so a SUCCESSOR prompt with the same window left `raw` holding the previous decision — the player could submit an amount they chose for a different prompt. Each reset now also keys on the identity its wire type carries: `source_id` for PayAmountChoice, `caster` for AssistPayment, and the pending cast's `object_id` for ChooseXValue. Each of the three regression tests holds the WINDOW CONSTANT so the identity field is the only changing dependency — the one shape in which the bug is observable — and each is proven by a mutant that removes just that dependency and reds that row by name. Residual, disclosed rather than papered over: a successor with the SAME identity and the same bounds is still indistinguishable here, because `WaitingFor` carries no prompt id or sequence. Closing that needs an identity emitted by the engine; deriving one in the display layer would be exactly the "frontend computes game state" mistake CLAUDE.md prohibits, so it is left to a follow-up rather than guessed at here. The steppers were 36px against the repo's 44pt touch-target rule (`.coderabbit.yaml`, `index.css`), and they are the only non-typing recovery path out of an invalid entry — the control a coarse pointer needs most. The 36px visual box is KEPT, because every other small square control in this client is `h-9 w-9` and resizing would both break that consistency and move the panel layout; the touch target is widened to 44px with the codebase's existing hit-area idiom (`ManualManaToggle`): a transparent `::before` at `-inset-1`, adding 4px per side. No test asserts the rendered hit box — happy-dom performs no layout — so this is a CSS-only change verified by reading, consistent with the existing #2427 disclaimer in this suite. Assisted-by: ClaudeCode:claude-opus-5 --- client/src/components/mana/AmountInput.tsx | 16 ++++++- .../src/components/mana/AssistPaymentUI.tsx | 8 +++- client/src/components/mana/ChooseXValueUI.tsx | 5 ++- .../src/components/mana/PayAmountChoiceUI.tsx | 5 ++- .../mana/__tests__/AssistPaymentUI.test.tsx | 30 +++++++++++++ .../mana/__tests__/ChooseXValueUI.test.tsx | 42 +++++++++++++++++++ .../mana/__tests__/PayAmountChoiceUI.test.tsx | 34 +++++++++++++++ 7 files changed, 135 insertions(+), 5 deletions(-) diff --git a/client/src/components/mana/AmountInput.tsx b/client/src/components/mana/AmountInput.tsx index cc49d56a1c..0826ba2a1e 100644 --- a/client/src/components/mana/AmountInput.tsx +++ b/client/src/components/mana/AmountInput.tsx @@ -99,7 +99,13 @@ export function AmountInput({ tone: "neutral", size: "xs", disabled: decDisabled, - className: "h-9 w-9 px-0 text-base", + // The 36px visual box is kept (every other small square control in this client is + // `h-9 w-9`), and the TOUCH target is widened to 44px with the codebase's existing + // hit-area idiom (`ManualManaToggle`): a transparent `::before` at `-inset-1` adds + // 4px per side. These steppers are the only non-typing recovery path out of an + // invalid entry, so they are exactly the control a coarse pointer needs most. + className: + "relative h-9 w-9 px-0 text-base before:absolute before:-inset-1 before:content-['']", })} > − @@ -151,7 +157,13 @@ export function AmountInput({ tone: "neutral", size: "xs", disabled: incDisabled, - className: "h-9 w-9 px-0 text-base", + // The 36px visual box is kept (every other small square control in this client is + // `h-9 w-9`), and the TOUCH target is widened to 44px with the codebase's existing + // hit-area idiom (`ManualManaToggle`): a transparent `::before` at `-inset-1` adds + // 4px per side. These steppers are the only non-typing recovery path out of an + // invalid entry, so they are exactly the control a coarse pointer needs most. + className: + "relative h-9 w-9 px-0 text-base before:absolute before:-inset-1 before:content-['']", })} > + diff --git a/client/src/components/mana/AssistPaymentUI.tsx b/client/src/components/mana/AssistPaymentUI.tsx index 0f3b90c7af..06c70690d4 100644 --- a/client/src/components/mana/AssistPaymentUI.tsx +++ b/client/src/components/mana/AssistPaymentUI.tsx @@ -22,11 +22,17 @@ export function AssistPaymentUI() { const isAssistPayment = waitingFor?.type === "AssistPayment"; const max = isAssistPayment ? waitingFor.data.max_generic : 0; + // Prompt identity, not just bounds: a successor prompt with the SAME max would otherwise leave + // `raw` holding the previous decision, letting the player submit an amount they chose for a + // different prompt. `caster` is the only identity this wire type carries. See the note in + // `AmountInput` — a same-caster/same-max successor is still indistinguishable here, and closing + // that needs a prompt identity from the engine rather than a guess in the display layer. + const caster = isAssistPayment ? waitingFor.data.caster : null; const [raw, setRaw] = useState("0"); useEffect(() => { if (isAssistPayment) setRaw("0"); - }, [isAssistPayment, max]); + }, [isAssistPayment, max, caster]); // CR 702.132a: "the player you chose may pay for any amount of the generic mana in the // spell's total cost" — the variant's domain is 0..max_generic. This is NOT a diff --git a/client/src/components/mana/ChooseXValueUI.tsx b/client/src/components/mana/ChooseXValueUI.tsx index d318b9cfab..3523fab01b 100644 --- a/client/src/components/mana/ChooseXValueUI.tsx +++ b/client/src/components/mana/ChooseXValueUI.tsx @@ -56,12 +56,15 @@ export function ChooseXValueUI() { useEffect(() => { if (isChooseX) setRaw(String(defaultValue)); + // `pendingCast?.object_id` keys the reset on prompt IDENTITY, not just its window: a + // successor ChooseXValue for a DIFFERENT spell with the same [min, max] would otherwise + // leave `raw` holding the X chosen for the previous spell. // `max` is a dependency even though it does not appear in the body: re-entering ChooseXValue // with a NARROWER max but an unchanged min leaves `defaultValue` identical, so without it the // effect would not fire and a now-out-of-range entry would persist with no way to self-heal. // Both sibling prompts already key their reset on the full window (PayAmountChoiceUI on // [min, max], AssistPaymentUI on [max]); this closes that asymmetry. - }, [isChooseX, defaultValue, max]); + }, [isChooseX, defaultValue, max, pendingCast?.object_id]); const handleCommit = useCallback(() => { // Sanitization gate: an out-of-range X is REJECTED, not clamped. Typing 99 under max=5 diff --git a/client/src/components/mana/PayAmountChoiceUI.tsx b/client/src/components/mana/PayAmountChoiceUI.tsx index b764f6084e..d9220114ef 100644 --- a/client/src/components/mana/PayAmountChoiceUI.tsx +++ b/client/src/components/mana/PayAmountChoiceUI.tsx @@ -18,11 +18,14 @@ export function PayAmountChoiceUI() { const data = isPayAmount ? waitingFor.data : null; const min = data?.min ?? 0; const max = data?.max ?? 0; + // Prompt identity, not just bounds — a successor prompt with the same window would otherwise + // leave `raw` holding the amount chosen for the PREVIOUS prompt. + const sourceId = data?.source_id ?? null; const [raw, setRaw] = useState(String(min)); useEffect(() => { if (isPayAmount) setRaw(String(min)); - }, [isPayAmount, min, max]); + }, [isPayAmount, min, max, sourceId]); const amount = parseAmount(raw, min, max); diff --git a/client/src/components/mana/__tests__/AssistPaymentUI.test.tsx b/client/src/components/mana/__tests__/AssistPaymentUI.test.tsx index c5404430fa..b6d4b29035 100644 --- a/client/src/components/mana/__tests__/AssistPaymentUI.test.tsx +++ b/client/src/components/mana/__tests__/AssistPaymentUI.test.tsx @@ -69,6 +69,36 @@ describe("AssistPaymentUI", () => { expect(screen.getByLabelText("Decrease amount")).toBeDisabled(); }); + // `max_generic` is held CONSTANT so the caster is the only changing dep: keyed on the bound + // alone the effect would not fire, and the amount chosen for the previous assist prompt would + // carry into this one. Residual, disclosed in the source: a successor with the SAME caster and + // bound is still indistinguishable — this wire type carries no prompt identity, and inventing + // one in the display layer would be the wrong fix. + it("AP/successor: a same-bound prompt for a different caster resets the entry", () => { + const promptFor = (caster: number) => + buildAssistPaymentWaitingFor({ data: { caster, chosen: 0, max_generic: 4 } }); + + const first = promptFor(1); + setGameStoreForTest({ + gameState: createGameState({ waiting_for: first }), + waitingFor: first, + }); + + const { rerender } = render(); + const box = screen.getByLabelText("Assist: Pay Generic Mana"); + fireEvent.change(box, { target: { value: "3" } }); + expect(box).toHaveValue("3"); + + const successor = promptFor(2); + setGameStoreForTest({ + gameState: createGameState({ waiting_for: successor }), + waitingFor: successor, + }); + rerender(); + + expect(screen.getByLabelText("Assist: Pay Generic Mana")).toHaveValue("0"); + }); + it("dispatches CommitAssistPayment with the selected value", () => { const dispatch = vi.fn().mockResolvedValue([]); const waitingFor = assistPaymentWaitingFor(4); diff --git a/client/src/components/mana/__tests__/ChooseXValueUI.test.tsx b/client/src/components/mana/__tests__/ChooseXValueUI.test.tsx index 257fb43d45..a1bbaaa937 100644 --- a/client/src/components/mana/__tests__/ChooseXValueUI.test.tsx +++ b/client/src/components/mana/__tests__/ChooseXValueUI.test.tsx @@ -330,6 +330,48 @@ describe("ChooseXValueUI", () => { ).toBeInTheDocument(); }); + // Companion to the `max` row above, for the other identity axis: the window is held CONSTANT + // and only the SPELL changes, so `pending_cast.object_id` is the sole changing dep. Keyed on + // bounds alone, the X chosen for the previous spell would carry into the next one. + it("resets when a same-window ChooseXValue arrives for a different spell", () => { + const promptFor = (objectId: number) => + buildChooseXValueWaitingFor({ + data: { + player: 0, + max: 10, + min: 0, + pending_cast: buildPendingCast({ + object_id: objectId, + card_id: 1, + cost: { type: "Cost", shards: ["X", "G"], generic: 0 }, + }), + }, + }); + + const first = promptFor(42); + setGameStoreForTest({ + gameState: createGameState({ waiting_for: first }), + waitingFor: first, + }); + + const { rerender } = render(); + fireEvent.change(screen.getByLabelText("Enter X value"), { + target: { value: "7" }, + }); + expect( + screen.getByRole("button", { name: "Confirm X = 7" }), + ).toBeInTheDocument(); + + const successor = promptFor(43); + setGameStoreForTest({ + gameState: createGameState({ waiting_for: successor }), + waitingFor: successor, + }); + rerender(); + + expect(screen.getByLabelText("Enter X value")).toHaveValue("0"); + }); + it("renders nothing for impossible min greater than max bounds", () => { const waitingFor = chooseXWaitingFor(0, 1); diff --git a/client/src/components/mana/__tests__/PayAmountChoiceUI.test.tsx b/client/src/components/mana/__tests__/PayAmountChoiceUI.test.tsx index fd66a26aef..fcf0ef34c8 100644 --- a/client/src/components/mana/__tests__/PayAmountChoiceUI.test.tsx +++ b/client/src/components/mana/__tests__/PayAmountChoiceUI.test.tsx @@ -208,6 +208,40 @@ describe("PayAmountChoiceUI — sanitized amount entry", () => { expect(dispatch).not.toHaveBeenCalled(); }); + // Reset is keyed on prompt IDENTITY, not just its window. Keyed on [min, max] alone the effect + // would not fire for a successor prompt with the same bounds, and the previous decision would + // carry into it. `min`/`max` are held CONSTANT here so `source_id` is the only changing dep — + // the one shape in which the identity dependency is observable. + it("T10/successor: a same-window prompt from a DIFFERENT source resets the entry", () => { + const promptFrom = (sourceId: number) => + buildPayAmountChoiceWaitingFor({ + data: { + player: 0, + resource: { type: "Counters" }, + min: 0, + max: 10, + source_id: sourceId, + }, + }); + + const { rerender } = renderPrompt(promptFrom(1)); + type("7"); + expect(screen.getByLabelText(BOX)).toHaveValue("7"); + + const successor = promptFrom(2); + setGameStoreForTest({ + gameState: buildGameState({ + waiting_for: successor, + active_player: 0, + turn_decision_controller: 0, + }), + waitingFor: successor, + }); + rerender(); + + expect(screen.getByLabelText(BOX)).toHaveValue("0"); + }); + it("T6a/enter-valid: Enter submits a valid entry", () => { const { dispatch } = renderPrompt(loopCollapseWaitingFor("Tokens")); type("42"); From 95af45c837d4135f9dac522a4ff862ae27c8cafc Mon Sep 17 00:00:00 2001 From: lgray Date: Wed, 5 Aug 2026 00:46:30 -0500 Subject: [PATCH 5/9] fix(client): real 44px targets, and key resets on the acting seat too MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Review of the previous commit found both of its fixes incomplete. Both are mine. The stepper hit area measured 42px, not the 44px its own comment claimed. `gameButtonClass` includes `border`, and an absolutely positioned pseudo-element resolves against its ancestor's PADDING box (36 − 2×1 = 34), so `before:-inset-1` yields 34 + 8 = 42. The idiom works in `board/ManualManaToggle` only because that control uses `ring-1`, which adds no layout border — so the precedent I cited did not transfer. Rather than retune the inset, the pseudo-element is DELETED and the controls are simply `h-11` (44px). A size that has to be derived through two CSS rules to be checked is a size that will silently regress; this one already had. The numeric box gets `h-11` too — it is the primary tap target, and fixing only the steppers left the main control short. It could not have used the same trick anyway: `` is a replaced element and renders no pseudo-elements. The identity deps omitted the acting seat, which is exactly the axis the engine varies. `effects/pay.rs` drives `PlayerFilter::All` and its own test asserts consecutive PayAmountChoice states with a constant `source_id` and `player` 0 then 1; on the life arm, two seats at equal life produce successive prompts whose `min`, `max` and `source_id` are all identical. `AssistPayment` likewise carries `chosen` — the seat actually asked to pay — alongside `caster`, so the previous comment's claim that `caster` was "the only identity this wire type carries" was false. Both dep arrays now include the seat. Both new rows use the CR 723.1a control shape (the local seat stays `turn_decision_controller` while the prompt's semantic player moves), which is what lets ONE client answer both prompts in a row and is therefore what makes the bug reachable. That shape was not guessed: setting the controller to the new seat as well hides the panel and the row fails, which is how it was pinned. Each row reds under a mutant removing only its own dependency. This also narrows the previous commit's residual claim: for PayAmountChoice the engine already emits a per-prompt-constant field that closes the reachable case, so "needs an engine-emitted identity" was too broad. What remains unclosable in the display layer is a successor identical in seat, source and bounds. Assisted-by: ClaudeCode:claude-opus-5 --- client/src/components/mana/AmountInput.tsx | 39 +++++++++------- .../src/components/mana/AssistPaymentUI.tsx | 9 ++-- .../src/components/mana/PayAmountChoiceUI.tsx | 9 +++- .../mana/__tests__/AssistPaymentUI.test.tsx | 33 ++++++++++++++ .../mana/__tests__/PayAmountChoiceUI.test.tsx | 45 +++++++++++++++++++ 5 files changed, 114 insertions(+), 21 deletions(-) diff --git a/client/src/components/mana/AmountInput.tsx b/client/src/components/mana/AmountInput.tsx index 0826ba2a1e..61721cd8f9 100644 --- a/client/src/components/mana/AmountInput.tsx +++ b/client/src/components/mana/AmountInput.tsx @@ -99,13 +99,15 @@ export function AmountInput({ tone: "neutral", size: "xs", disabled: decDisabled, - // The 36px visual box is kept (every other small square control in this client is - // `h-9 w-9`), and the TOUCH target is widened to 44px with the codebase's existing - // hit-area idiom (`ManualManaToggle`): a transparent `::before` at `-inset-1` adds - // 4px per side. These steppers are the only non-typing recovery path out of an - // invalid entry, so they are exactly the control a coarse pointer needs most. - className: - "relative h-9 w-9 px-0 text-base before:absolute before:-inset-1 before:content-['']", + // 44px REAL size, not a 36px box with an expanded `::before`. The pseudo-element + // trick (as in `board/ManualManaToggle`) measured 42px here, not 44: `gameButtonClass` + // adds `border`, and an absolutely positioned pseudo resolves against its ancestor's + // PADDING box (36 − 2×1 = 34), so `-inset-1` yields 34 + 8. It works in + // `ManualManaToggle` only because that control uses `ring-1`, which adds no layout + // border. A size that must be derived to be checked is a size that will silently + // regress; these steppers are the only non-typing recovery path out of an invalid + // entry, so they get the boring, directly-readable 44px. + className: "h-11 w-11 px-0 text-base", })} > − @@ -141,8 +143,11 @@ export function AmountInput({ // natively; a text box announces nothing, so the range hint is permanently associated // and the error message is appended to it while invalid. aria-describedby={amount === null ? `${hintId} ${errorId}` : hintId} - // `w-24` not `w-20`: four digits must fit for the 1000 case. - className={`h-9 w-24 rounded-lg border bg-gray-950/80 px-2 text-center font-mono text-base font-semibold shadow-inner outline-none transition focus:ring-2 ${ + // `w-24` not `w-20`: four digits must fit for the 1000 case. `h-11` (44px) because this + // is the PRIMARY tap target of the control — fixing only the steppers would leave the + // main one short. The `::before` idiom cannot be used here regardless: `` is a + // replaced element and renders no pseudo-elements. + className={`h-11 w-24 rounded-lg border bg-gray-950/80 px-2 text-center font-mono text-base font-semibold shadow-inner outline-none transition focus:ring-2 ${ amount === null ? "border-red-400/60 text-red-200 focus:ring-red-400/30" : "border-cyan-400/30 text-cyan-100 focus:ring-cyan-400/30" @@ -157,13 +162,15 @@ export function AmountInput({ tone: "neutral", size: "xs", disabled: incDisabled, - // The 36px visual box is kept (every other small square control in this client is - // `h-9 w-9`), and the TOUCH target is widened to 44px with the codebase's existing - // hit-area idiom (`ManualManaToggle`): a transparent `::before` at `-inset-1` adds - // 4px per side. These steppers are the only non-typing recovery path out of an - // invalid entry, so they are exactly the control a coarse pointer needs most. - className: - "relative h-9 w-9 px-0 text-base before:absolute before:-inset-1 before:content-['']", + // 44px REAL size, not a 36px box with an expanded `::before`. The pseudo-element + // trick (as in `board/ManualManaToggle`) measured 42px here, not 44: `gameButtonClass` + // adds `border`, and an absolutely positioned pseudo resolves against its ancestor's + // PADDING box (36 − 2×1 = 34), so `-inset-1` yields 34 + 8. It works in + // `ManualManaToggle` only because that control uses `ring-1`, which adds no layout + // border. A size that must be derived to be checked is a size that will silently + // regress; these steppers are the only non-typing recovery path out of an invalid + // entry, so they get the boring, directly-readable 44px. + className: "h-11 w-11 px-0 text-base", })} > + diff --git a/client/src/components/mana/AssistPaymentUI.tsx b/client/src/components/mana/AssistPaymentUI.tsx index 06c70690d4..0d9323066b 100644 --- a/client/src/components/mana/AssistPaymentUI.tsx +++ b/client/src/components/mana/AssistPaymentUI.tsx @@ -24,15 +24,16 @@ export function AssistPaymentUI() { const max = isAssistPayment ? waitingFor.data.max_generic : 0; // Prompt identity, not just bounds: a successor prompt with the SAME max would otherwise leave // `raw` holding the previous decision, letting the player submit an amount they chose for a - // different prompt. `caster` is the only identity this wire type carries. See the note in - // `AmountInput` — a same-caster/same-max successor is still indistinguishable here, and closing - // that needs a prompt identity from the engine rather than a guess in the display layer. + // different prompt. BOTH seats are keyed on, not just `caster` — `chosen` is the seat actually + // being asked to pay, so one caster polling two different players in succession changes only + // `chosen`, and keying on `caster` alone would not fire. const caster = isAssistPayment ? waitingFor.data.caster : null; + const chosen = isAssistPayment ? waitingFor.data.chosen : null; const [raw, setRaw] = useState("0"); useEffect(() => { if (isAssistPayment) setRaw("0"); - }, [isAssistPayment, max, caster]); + }, [isAssistPayment, max, caster, chosen]); // CR 702.132a: "the player you chose may pay for any amount of the generic mana in the // spell's total cost" — the variant's domain is 0..max_generic. This is NOT a diff --git a/client/src/components/mana/PayAmountChoiceUI.tsx b/client/src/components/mana/PayAmountChoiceUI.tsx index d9220114ef..ed6c6c7307 100644 --- a/client/src/components/mana/PayAmountChoiceUI.tsx +++ b/client/src/components/mana/PayAmountChoiceUI.tsx @@ -20,12 +20,19 @@ export function PayAmountChoiceUI() { const max = data?.max ?? 0; // Prompt identity, not just bounds — a successor prompt with the same window would otherwise // leave `raw` holding the amount chosen for the PREVIOUS prompt. + // `player` as well as `source_id`: the engine PROVABLY emits consecutive PayAmountChoice states + // with a constant `source_id` and a changing `player` — `effects/pay.rs` drives + // `PlayerFilter::All` and its own test asserts prompt 1 `player=0` then prompt 2 `player=1` with + // no intervening `WaitingFor`. On the life arm, two seats at equal life produce successive + // prompts whose `min`, `max` AND `source_id` are all identical, so `source_id` alone would not + // fire and the second seat would inherit the first seat's typed amount. const sourceId = data?.source_id ?? null; + const promptPlayer = data?.player ?? null; const [raw, setRaw] = useState(String(min)); useEffect(() => { if (isPayAmount) setRaw(String(min)); - }, [isPayAmount, min, max, sourceId]); + }, [isPayAmount, min, max, sourceId, promptPlayer]); const amount = parseAmount(raw, min, max); diff --git a/client/src/components/mana/__tests__/AssistPaymentUI.test.tsx b/client/src/components/mana/__tests__/AssistPaymentUI.test.tsx index b6d4b29035..c54e115d45 100644 --- a/client/src/components/mana/__tests__/AssistPaymentUI.test.tsx +++ b/client/src/components/mana/__tests__/AssistPaymentUI.test.tsx @@ -99,6 +99,39 @@ describe("AssistPaymentUI", () => { expect(screen.getByLabelText("Assist: Pay Generic Mana")).toHaveValue("0"); }); + // The seat axis for assist: same caster, same bound, different payer. Uses the CR 723.1a shape + // (local seat 0 stays `turn_decision_controller` while the semantic player moves) so one client + // renders both prompts in a row — otherwise the panel hides and the row would be vacuous. + it("AP/successor-seat: a same-caster prompt for a different PAYER resets the entry", () => { + const promptFor = (chosen: number) => + buildAssistPaymentWaitingFor({ data: { caster: 1, chosen, max_generic: 4 } }); + + const seat = (waitingFor: WaitingFor, active: number) => { + setGameStoreForTest({ + gameState: createGameState({ + waiting_for: waitingFor, + active_player: active, + turn_decision_controller: 0, + }), + waitingFor, + }); + }; + + const first = promptFor(0); + seat(first, 0); + const { rerender } = render(); + fireEvent.change(screen.getByLabelText("Assist: Pay Generic Mana"), { + target: { value: "3" }, + }); + expect(screen.getByLabelText("Assist: Pay Generic Mana")).toHaveValue("3"); + + const second = promptFor(1); + seat(second, 1); + rerender(); + + expect(screen.getByLabelText("Assist: Pay Generic Mana")).toHaveValue("0"); + }); + it("dispatches CommitAssistPayment with the selected value", () => { const dispatch = vi.fn().mockResolvedValue([]); const waitingFor = assistPaymentWaitingFor(4); diff --git a/client/src/components/mana/__tests__/PayAmountChoiceUI.test.tsx b/client/src/components/mana/__tests__/PayAmountChoiceUI.test.tsx index fcf0ef34c8..94228c3bd9 100644 --- a/client/src/components/mana/__tests__/PayAmountChoiceUI.test.tsx +++ b/client/src/components/mana/__tests__/PayAmountChoiceUI.test.tsx @@ -242,6 +242,51 @@ describe("PayAmountChoiceUI — sanitized amount entry", () => { expect(screen.getByLabelText(BOX)).toHaveValue("0"); }); + // The seat axis, which `source_id` alone does NOT cover. `effects/pay.rs` drives + // `PlayerFilter::All` and its own test asserts consecutive PayAmountChoice states with a + // constant source and `player` 0 then 1; on the life arm two seats at equal life make `min`, + // `max` and `source_id` all identical, so the seat is the only thing that changes. + it("T11/successor-seat: a same-source prompt for a DIFFERENT seat resets the entry", () => { + const promptFor = (player: number) => + buildPayAmountChoiceWaitingFor({ + data: { + player, + resource: { type: "Counters" }, + min: 0, + max: 10, + source_id: 9, + }, + }); + + // CR 723.1a control is what makes this reachable in ONE client: the local seat (0) stays the + // `turn_decision_controller` while the prompt's semantic player moves to seat 1, so + // `useCanActForWaitingState` (usePlayerId.ts:95) keeps rendering and the same client answers + // both prompts in a row. Setting the controller to 1 as well would hide the panel and make + // this row vacuous — it fails that way, which is how the shape was pinned. + const seat = (waitingFor: WaitingFor, player: number) => { + setGameStoreForTest({ + gameState: buildGameState({ + waiting_for: waitingFor, + active_player: player, + turn_decision_controller: 0, + }), + waitingFor, + }); + }; + + const first = promptFor(0); + seat(first, 0); + const { rerender } = render(); + type("7"); + expect(screen.getByLabelText(BOX)).toHaveValue("7"); + + const second = promptFor(1); + seat(second, 1); + rerender(); + + expect(screen.getByLabelText(BOX)).toHaveValue("0"); + }); + it("T6a/enter-valid: Enter submits a valid entry", () => { const { dispatch } = renderPrompt(loopCollapseWaitingFor("Tokens")); type("42"); From 45a59914282f6b3e739e14209f2aacd3d5bdf108 Mon Sep 17 00:00:00 2001 From: lgray Date: Wed, 5 Aug 2026 01:01:53 -0500 Subject: [PATCH 6/9] fix(client): cite CR 723.5 for turn control, and drop two stale claims MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Four review findings, all mine. The CR citation was wrong. I cited CR 723.1a for "the controlling seat makes the controlled seat's decisions", but 723.1a is "multiple player-controlling effects that affect the same player overwrite each other". The rule that governs the claim is CR 723.5 — "while controlling another player, a player makes all choices and decisions the controlled player is allowed to make" — paired with CR 723.3, "a player who's being controlled during their turn is still the active player", which is why the active player moves while the controller does not. Both were grepped from `docs/MagicCompRules.txt` before writing, and CR 723.5 is already what the rest of this repo uses for this exact seam (`game/autoPass.ts`, `adapter/p2p-adapter.ts`, `engine/src/game/turns.rs`). This is the sixth time in this workstream that a real rule has been cited for a claim it does not govern, and the second time I have done it while fixing an instance of it. The recurring shape is that the citation is checked for existence, not for whether it reaches the case. A note on the sibling assist row still said a same-caster successor was indistinguishable and that this wire type carries no prompt identity — which the previous commit disproved by adding `chosen`. Deleting code and adding fields both invalidate comments that point at them; the stale note is corrected rather than left to mislead someone into dropping the dep. The residual claim is narrowed a second time. `PayAmountChoice` also carries `accumulated` (stamped on every successor by `finish_pay_amount_choice`) and `resource`; neither is in the dep array because no reachable path needs them, but "identical in seat, source and bounds" overstated what is actually indistinguishable. `px-0` on the steppers was dead: `SIZE_CLASSES.xs` emits `px-2.5` later in the compiled sheet and wins. Removed rather than left reading as load-bearing; `w-11` pins the 44px border box regardless of padding. Assisted-by: ClaudeCode:claude-opus-5 --- client/src/components/mana/AmountInput.tsx | 10 ++++++++-- .../mana/__tests__/AssistPaymentUI.test.tsx | 13 +++++++------ .../mana/__tests__/PayAmountChoiceUI.test.tsx | 12 +++++++----- 3 files changed, 22 insertions(+), 13 deletions(-) diff --git a/client/src/components/mana/AmountInput.tsx b/client/src/components/mana/AmountInput.tsx index 61721cd8f9..258685dac4 100644 --- a/client/src/components/mana/AmountInput.tsx +++ b/client/src/components/mana/AmountInput.tsx @@ -107,7 +107,10 @@ export function AmountInput({ // border. A size that must be derived to be checked is a size that will silently // regress; these steppers are the only non-typing recovery path out of an invalid // entry, so they get the boring, directly-readable 44px. - className: "h-11 w-11 px-0 text-base", + // No `px-0`: `SIZE_CLASSES.xs` emits `px-2.5` later in the compiled sheet and wins, + // so a `px-0` here would read as load-bearing while doing nothing. `w-11` pins the + // border box at 44px regardless of padding (border-box sizing). + className: "h-11 w-11 text-base", })} > − @@ -170,7 +173,10 @@ export function AmountInput({ // border. A size that must be derived to be checked is a size that will silently // regress; these steppers are the only non-typing recovery path out of an invalid // entry, so they get the boring, directly-readable 44px. - className: "h-11 w-11 px-0 text-base", + // No `px-0`: `SIZE_CLASSES.xs` emits `px-2.5` later in the compiled sheet and wins, + // so a `px-0` here would read as load-bearing while doing nothing. `w-11` pins the + // border box at 44px regardless of padding (border-box sizing). + className: "h-11 w-11 text-base", })} > + diff --git a/client/src/components/mana/__tests__/AssistPaymentUI.test.tsx b/client/src/components/mana/__tests__/AssistPaymentUI.test.tsx index c54e115d45..8eec43675f 100644 --- a/client/src/components/mana/__tests__/AssistPaymentUI.test.tsx +++ b/client/src/components/mana/__tests__/AssistPaymentUI.test.tsx @@ -71,9 +71,9 @@ describe("AssistPaymentUI", () => { // `max_generic` is held CONSTANT so the caster is the only changing dep: keyed on the bound // alone the effect would not fire, and the amount chosen for the previous assist prompt would - // carry into this one. Residual, disclosed in the source: a successor with the SAME caster and - // bound is still indistinguishable — this wire type carries no prompt identity, and inventing - // one in the display layer would be the wrong fix. + // carry into this one. The PAYER axis is covered separately by AP/successor-seat below — an + // earlier version of this note claimed a same-caster successor was indistinguishable, which + // `chosen` disproves. it("AP/successor: a same-bound prompt for a different caster resets the entry", () => { const promptFor = (caster: number) => buildAssistPaymentWaitingFor({ data: { caster, chosen: 0, max_generic: 4 } }); @@ -99,9 +99,10 @@ describe("AssistPaymentUI", () => { expect(screen.getByLabelText("Assist: Pay Generic Mana")).toHaveValue("0"); }); - // The seat axis for assist: same caster, same bound, different payer. Uses the CR 723.1a shape - // (local seat 0 stays `turn_decision_controller` while the semantic player moves) so one client - // renders both prompts in a row — otherwise the panel hides and the row would be vacuous. + // The seat axis for assist: same caster, same bound, different payer. Uses the CR 723.5 + + // CR 723.3 control shape (the local seat stays `turn_decision_controller` while the semantic + // player moves) so one client renders both prompts in a row — otherwise the panel hides and + // the row would be vacuous, which is how the shape was pinned. it("AP/successor-seat: a same-caster prompt for a different PAYER resets the entry", () => { const promptFor = (chosen: number) => buildAssistPaymentWaitingFor({ data: { caster: 1, chosen, max_generic: 4 } }); diff --git a/client/src/components/mana/__tests__/PayAmountChoiceUI.test.tsx b/client/src/components/mana/__tests__/PayAmountChoiceUI.test.tsx index 94228c3bd9..8b174fe97e 100644 --- a/client/src/components/mana/__tests__/PayAmountChoiceUI.test.tsx +++ b/client/src/components/mana/__tests__/PayAmountChoiceUI.test.tsx @@ -258,11 +258,13 @@ describe("PayAmountChoiceUI — sanitized amount entry", () => { }, }); - // CR 723.1a control is what makes this reachable in ONE client: the local seat (0) stays the - // `turn_decision_controller` while the prompt's semantic player moves to seat 1, so - // `useCanActForWaitingState` (usePlayerId.ts:95) keeps rendering and the same client answers - // both prompts in a row. Setting the controller to 1 as well would hide the panel and make - // this row vacuous — it fails that way, which is how the shape was pinned. + // CR 723.5 ("while controlling another player, a player makes all choices and decisions the + // controlled player is allowed to make") + CR 723.3 ("a player who's being controlled during + // their turn is still the active player") — which together are why the local seat (0) stays + // `turn_decision_controller` while the prompt's semantic player moves to seat 1. That is what + // keeps `useCanActForWaitingState` (usePlayerId.ts:95) true, so ONE client answers both + // prompts in a row. Setting the controller to 1 as well hides the panel and makes this row + // vacuous — it fails that way, which is how the shape was pinned rather than guessed. const seat = (waitingFor: WaitingFor, player: number) => { setGameStoreForTest({ gameState: buildGameState({ From dbebc76d65ba24122e20cef1aaed8a1fa305aada Mon Sep 17 00:00:00 2001 From: lgray Date: Wed, 5 Aug 2026 01:10:19 -0500 Subject: [PATCH 7/9] test(client): pin that AmountInput does not re-guard Enter MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit CodeRabbit's review of #7019 named two uncovered behaviors in `AmountInput`. One was real; the other was not, and the difference was settled by mutation rather than by reading. The real one: `onSubmit` is handed in as `vi.fn()` by every row in the file and never asserted. The caller suites (T6b, AP/enter) do cover Enter, but only through the assertion that an invalid entry never reaches the engine — which is satisfied EQUALLY by "onSubmit was never called" and by "onSubmit was called and the caller's guard rejected it". Nothing in the suite separated those, so the `onSubmit` prop comment ("MUST itself reject an invalid amount — AmountInput deliberately does not re-guard") was an assertion in prose only. Re-adding the forbidden guard inside the component now reds exactly one row, this new one. The one that was not real: a `min > 0` hint row. Both legs are already dominated. Collapsing the ternary to the max-only string reds `ChooseXValueUI`'s "min 1 / max 10"; collapsing it the other way reds three rows, because each of them asserts the hint's FULL text rather than a substring. The candidate row added no fourth failure. It is left out, with the measurement recorded where the next reader (or the next bot) will look for it — a row that cannot fail is worse than no row, because it reads as coverage. Assisted-by: ClaudeCode:claude-opus-5 --- .../mana/__tests__/AmountInput.test.tsx | 43 +++++++++++++++++++ 1 file changed, 43 insertions(+) diff --git a/client/src/components/mana/__tests__/AmountInput.test.tsx b/client/src/components/mana/__tests__/AmountInput.test.tsx index efd4c61d92..b2f32c196c 100644 --- a/client/src/components/mana/__tests__/AmountInput.test.tsx +++ b/client/src/components/mana/__tests__/AmountInput.test.tsx @@ -280,4 +280,47 @@ describe("AmountInput — rendered contract", () => { fireEvent.click(screen.getByLabelText("Increase amount")); expect(onRawChange).toHaveBeenLastCalledWith("1000"); }); + + // The Enter route at the BUILDING-BLOCK level. The caller suites (T6b, AP/enter) assert only + // that an invalid entry never reaches the engine — and that is satisfied EQUALLY by "onSubmit + // was never called" and by "onSubmit was called and the caller's guard rejected it". Nothing + // separated those two until this row, which is what turns the `onSubmit` prop comment ("MUST + // itself reject an invalid amount — AmountInput deliberately does not re-guard") from an + // assertion in prose into one the suite checks. The second half is the discriminating half: + // re-adding a guard here is exactly the change the comment forbids, and it reds only this row. + it("enter: Enter calls onSubmit, and calls it EVEN when the entry is out of range", () => { + const onSubmit = vi.fn(); + const { rerender } = render( + , + ); + fireEvent.keyDown(screen.getByLabelText("Enter amount"), { key: "Enter" }); + expect(onSubmit).toHaveBeenCalledTimes(1); + + rerender( + , + ); + fireEvent.keyDown(screen.getByLabelText("Enter amount"), { key: "Enter" }); + expect(onSubmit).toHaveBeenCalledTimes(2); + }); + + // ponytail: no `min > 0` hint row here. A review asked for one on the premise that the branch + // was uncovered; MEASURED, both legs are already dominated, so the row would detect nothing. + // Collapsing the ternary to the max-only string reds `ChooseXValueUI`'s "min 1 / max 10"; + // collapsing it to the two-sided string reds three rows (this file's A11Y/associate equality on + // "max 5", the assist suite's "max 4", and a ChooseXValue row) because each asserts the hint's + // FULL text, not a substring. A candidate row asserting both legs failed to add a fourth. }); From e7b3a1fa1c1261f1bfac9eefec13667ab8a4dc30 Mon Sep 17 00:00:00 2001 From: lgray Date: Wed, 5 Aug 2026 01:14:55 -0500 Subject: [PATCH 8/9] docs(client): repoint the reset-dep evidence at a case that actually shows it MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Comment-only. Three review findings, all of them "the claim is broader than the measurement behind it". The `player` dep was justified by an equal-life case on the pay arm that I never demonstrated. `effects/pay.rs` does drive `PlayerFilter::All`, and its own test does assert consecutive `PayAmountChoice` states with a constant `source_id` and `player` 0 then 1 — but `max` and `accumulated` move in that case too, so keying on `[min, max]` alone would already have fired. The test I cited does not show what I said it showed, and the word "PROVABLY" was carrying the gap. The case that does show it is the LoopCollapse arm in `game/turns.rs` — which is the prompt this component's own tests already model. It mints one prompt per controller in APNAP order with `min: 0`, `accumulated: 0`, `source_id: ObjectId(0)` and `pending_mana_ability: None` written as literals: constant by construction, not conditional on board state. Only `max` and the collapse axis vary per controller, so two controllers with equal counts on the same axis differ in `player` alone. Read off the field initializers rather than inferred. The sibling parenthetical in `ChooseXValueUI` enumerated the two other dep arrays as `[min, max]` and `[max]` — which the immediately preceding commit falsified by adding the seat fields to both. An enumeration that reads as exhaustive and is not is worse than naming the shape, so it now names the shape. Assisted-by: ClaudeCode:claude-opus-5 --- client/src/components/mana/ChooseXValueUI.tsx | 6 ++++-- .../src/components/mana/PayAmountChoiceUI.tsx | 20 +++++++++++++------ 2 files changed, 18 insertions(+), 8 deletions(-) diff --git a/client/src/components/mana/ChooseXValueUI.tsx b/client/src/components/mana/ChooseXValueUI.tsx index 3523fab01b..88bb201540 100644 --- a/client/src/components/mana/ChooseXValueUI.tsx +++ b/client/src/components/mana/ChooseXValueUI.tsx @@ -62,8 +62,10 @@ export function ChooseXValueUI() { // `max` is a dependency even though it does not appear in the body: re-entering ChooseXValue // with a NARROWER max but an unchanged min leaves `defaultValue` identical, so without it the // effect would not fire and a now-out-of-range entry would persist with no way to self-heal. - // Both sibling prompts already key their reset on the full window (PayAmountChoiceUI on - // [min, max], AssistPaymentUI on [max]); this closes that asymmetry. + // Both sibling prompts already key their reset on their full window plus their own identity + // fields; this closes that asymmetry. (An earlier version enumerated those arrays here — the + // same commit that added the seat deps made the enumeration wrong, so it names the shape + // instead of the members.) }, [isChooseX, defaultValue, max, pendingCast?.object_id]); const handleCommit = useCallback(() => { diff --git a/client/src/components/mana/PayAmountChoiceUI.tsx b/client/src/components/mana/PayAmountChoiceUI.tsx index ed6c6c7307..c560f9b81c 100644 --- a/client/src/components/mana/PayAmountChoiceUI.tsx +++ b/client/src/components/mana/PayAmountChoiceUI.tsx @@ -20,12 +20,20 @@ export function PayAmountChoiceUI() { const max = data?.max ?? 0; // Prompt identity, not just bounds — a successor prompt with the same window would otherwise // leave `raw` holding the amount chosen for the PREVIOUS prompt. - // `player` as well as `source_id`: the engine PROVABLY emits consecutive PayAmountChoice states - // with a constant `source_id` and a changing `player` — `effects/pay.rs` drives - // `PlayerFilter::All` and its own test asserts prompt 1 `player=0` then prompt 2 `player=1` with - // no intervening `WaitingFor`. On the life arm, two seats at equal life produce successive - // prompts whose `min`, `max` AND `source_id` are all identical, so `source_id` alone would not - // fire and the second seat would inherit the first seat's typed amount. + // `player` as well as `source_id`: the engine emits consecutive PayAmountChoice states with a + // constant `source_id` and a changing `player`. `effects/pay.rs` drives `PlayerFilter::All` and + // its own test asserts prompt 1 `player=0` then prompt 2 `player=1` with no intervening + // `WaitingFor` — but there `max` and `accumulated` move too, so that case alone does not show + // `source_id` is insufficient. + // The case that does is the LoopCollapse arm in `game/turns.rs`, which is the prompt this + // component's own tests model: it mints one prompt per controller in APNAP order with + // `min: 0`, `accumulated: 0`, `source_id: ObjectId(0)` and `pending_mana_ability: None` all + // written as LITERALS — constant by construction, not conditional on board state. Only `max` + // (the controller's pending count) and the `LoopCollapse` axis vary, so two controllers with + // equal counts on the same axis produce successive prompts differing in `player` alone, and + // keying on `source_id` would leave the second controller holding the first one's typed amount. + // An earlier version of this note asserted an equal-life case on the pay arm instead; that path + // was never demonstrated, and this one is read straight off the field initializers. const sourceId = data?.source_id ?? null; const promptPlayer = data?.player ?? null; const [raw, setRaw] = useState(String(min)); From 895582315bcb1b640371c867b9c385d9bf20c700 Mon Sep 17 00:00:00 2001 From: lgray Date: Wed, 5 Aug 2026 01:31:19 -0500 Subject: [PATCH 9/9] docs(client): finish the sweep the previous commit started MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Comment-only. The previous commit fixed two claims in the component files and left the identical claims standing in their test files — so for one commit the repo asserted both that the equal-life pay-arm case proves the seat dependency and that it does not. Both swept now, by the defect's mechanism rather than by the file the review named: T11's justification repoints to the LoopCollapse arm of `game/turns.rs` (four of the seven prompt fields written as literals, so only `max` and the axis vary), and the sibling dep-array enumeration in the ChooseX suite names the shape instead of listing members that a later commit can falsify — which is exactly what happened to it. The third correction is to my own measurement write-up. The note explaining why the `min > 0` hint row was left out said a candidate row "failed to add a fourth" failing test. It did add one; what it failed to add was a DETECTION — every mutant it reds was already red elsewhere. The distinction is the whole point of calling a row dominated, and stating it as a failure count was both wrong and, worse, checkable-looking. Reworded to say what was measured. Assisted-by: ClaudeCode:claude-opus-5 --- .../components/mana/__tests__/AmountInput.test.tsx | 7 +++++-- .../mana/__tests__/ChooseXValueUI.test.tsx | 6 ++++-- .../mana/__tests__/PayAmountChoiceUI.test.tsx | 13 +++++++++---- 3 files changed, 18 insertions(+), 8 deletions(-) diff --git a/client/src/components/mana/__tests__/AmountInput.test.tsx b/client/src/components/mana/__tests__/AmountInput.test.tsx index b2f32c196c..1caf1eed7b 100644 --- a/client/src/components/mana/__tests__/AmountInput.test.tsx +++ b/client/src/components/mana/__tests__/AmountInput.test.tsx @@ -318,9 +318,12 @@ describe("AmountInput — rendered contract", () => { }); // ponytail: no `min > 0` hint row here. A review asked for one on the premise that the branch - // was uncovered; MEASURED, both legs are already dominated, so the row would detect nothing. + // was uncovered; MEASURED, both legs are already dominated, so the row would detect nothing NEW. // Collapsing the ternary to the max-only string reds `ChooseXValueUI`'s "min 1 / max 10"; // collapsing it to the two-sided string reds three rows (this file's A11Y/associate equality on // "max 5", the assist suite's "max 4", and a ChooseXValue row) because each asserts the hint's - // FULL text, not a substring. A candidate row asserting both legs failed to add a fourth. + // FULL text, not a substring. A candidate row asserting both legs does red alongside them — it + // is the two mutants' set of DETECTIONS it fails to grow, not their failure count. That is the + // definition of dominated, and it is why the row is absent: it would report coverage it is not + // supplying. Both mutants are one-line and re-runnable if you want to check the claim. }); diff --git a/client/src/components/mana/__tests__/ChooseXValueUI.test.tsx b/client/src/components/mana/__tests__/ChooseXValueUI.test.tsx index a1bbaaa937..2bfce9aa9f 100644 --- a/client/src/components/mana/__tests__/ChooseXValueUI.test.tsx +++ b/client/src/components/mana/__tests__/ChooseXValueUI.test.tsx @@ -292,8 +292,10 @@ describe("ChooseXValueUI", () => { // The test ABOVE cannot discriminate the reset's `max` dependency: it narrows min 1 → 2 as // well, so `defaultValue` changes and the effect fires either way. This row holds min CONSTANT, // which is the only shape in which a reset keyed on `defaultValue` alone is observably wrong. - // Both sibling prompts already key on the full window (PayAmountChoiceUI [min, max], - // AssistPaymentUI [max]) — this closes that asymmetry. + // Both sibling prompts already key on their full window plus their own identity fields — this + // closes that asymmetry. (This named the two dep arrays until the commit that added the seat + // fields made the enumeration wrong; it names the shape now, for the same reason the component's + // copy of this note does.) it("resets an entry stranded above a narrowed max when min is unchanged", () => { const dispatch = vi.fn().mockResolvedValue([]); const waitingFor = chooseXWaitingFor(10, 0); diff --git a/client/src/components/mana/__tests__/PayAmountChoiceUI.test.tsx b/client/src/components/mana/__tests__/PayAmountChoiceUI.test.tsx index 8b174fe97e..e7aa470307 100644 --- a/client/src/components/mana/__tests__/PayAmountChoiceUI.test.tsx +++ b/client/src/components/mana/__tests__/PayAmountChoiceUI.test.tsx @@ -242,10 +242,15 @@ describe("PayAmountChoiceUI — sanitized amount entry", () => { expect(screen.getByLabelText(BOX)).toHaveValue("0"); }); - // The seat axis, which `source_id` alone does NOT cover. `effects/pay.rs` drives - // `PlayerFilter::All` and its own test asserts consecutive PayAmountChoice states with a - // constant source and `player` 0 then 1; on the life arm two seats at equal life make `min`, - // `max` and `source_id` all identical, so the seat is the only thing that changes. + // The seat axis, which `source_id` alone does NOT cover. The reachable case is the LoopCollapse + // arm in `game/turns.rs` — the same prompt `loopCollapseWaitingFor` models here — which mints one + // prompt per controller in APNAP order with `min: 0`, `accumulated: 0`, `source_id: ObjectId(0)` + // and `pending_mana_ability: None` written as LITERALS. Only `max` and the collapse axis derive + // from state, so two controllers with equal counts on the same axis differ in `player` alone. + // (An earlier version cited an equal-life case on `effects/pay.rs` instead. That test does show + // consecutive prompts with a constant source and `player` 0 then 1 — but `max` and `accumulated` + // move there too, so `[min, max]` alone would already have fired and it proves nothing about + // `source_id` being insufficient. Same correction as in the component.) it("T11/successor-seat: a same-source prompt for a DIFFERENT seat resets the entry", () => { const promptFor = (player: number) => buildPayAmountChoiceWaitingFor({