feat(client): one sanitized numeric box for all three amount prompts

[lgray]

🤖 AI text below 🤖

Summary

Amount prompts (PayAmountChoice, ChooseXValue, Assist payment) currently commit whatever the widget yields, so a player can neither type a bound directly nor see why an out-of-range entry is refused. This replaces the slider/ad-hoc inputs with one shared, sanitized numeric box (AmountInput + parseAmount) adopted by all three prompts, with the engine-supplied [min, max] window announced rather than silently clamped.

Files changed

• client/src/components/mana/AmountInput.tsx — new; the shared box + parseAmount/digitsOf
• client/src/components/mana/__tests__/AmountInput.test.tsx — new
• client/src/components/mana/PayAmountChoiceUI.tsx — adopts the box (replaces the slider)
• client/src/components/mana/ChooseXValueUI.tsx — adopts the box
• client/src/components/mana/AssistPaymentUI.tsx — adopts the box
• client/src/components/mana/__tests__/PayAmountChoiceUI.test.tsx — rewritten for the box
• client/src/components/mana/__tests__/ChooseXValueUI.test.tsx — rewritten; three deliberate behaviour flips (below)
• client/src/components/mana/__tests__/AssistPaymentUI.test.tsx — rewritten for the box
• client/src/test/factories/gameStateFactory.ts — adds PayAmountChoiceWaitingForFactory (additive; buildGameState untouched)
• client/src/components/modal/DialogHost.tsx — comment only; repoints the Frontend: mana payment X-value input box unresponsive until clicking elsewhere on board #2427 guard's example at the surviving controls (see below)
• client/src/i18n/locales/{de,en,es,fr,it,pl,pt}/game.json — 4 mana.* additions (3 in the first commit, confirmAmount in the review-fix commit), 2 orphaned keys deleted

Track

Developer

LLM

Model: claude-opus-5
Tier: Frontier
Thinking: high

Implementation method (required)

Method: not-applicable — client-only. No crates/engine/ file is touched; there is no parser, effect, resolver, targeting or rules-behaviour change in this PR.

CR references

CR 702.132a — grep-verified verbatim against docs/MagicCompRules.txt before it was written; it authorises the Assist [0, max_generic] domain ("may pay for any amount of the generic mana in the spell's total cost"), which is why the absence of a min field is a real lower bound rather than a missing value.

Three CR citations in ChooseXValueUI.tsx are corrected, one of them mine. The final review round caught that the inline comment I added cited CR 601.2f for "a value the caster never chose" — but 601.2f is total cost determination. The rule that actually governs the claim is CR 107.3a: "the controller of that spell or ability chooses and announces the value of X as part of casting the spell", with CR 601.2b placing that announcement in the casting sequence. Grepping the rules text showed the same wrong citation already existed at two pre-existing sites in the same file (the JSDoc header and the guard comment above the early return), so all three are fixed rather than just mine — CLAUDE.md requires verifying existing CR annotations when modifying a file, and leaving the template in place is how the error propagates to the next reader. CR 107.1b is re-pointed at the bound it genuinely supports (a negative number can't be chosen ⇒ the lower bound is never below 0) instead of at cost determination. CR 601.2f survives only where the claim really is about the total cost being locked in.

Every rule number above was verified by grepping docs/MagicCompRules.txt before being written, and no MagicCompRules.txt line number is cited anywhere.

Verification

• Required checks ran clean, or the exact CI-owned alternative is stated below.
• Gate A output below is for the current committed head.
• Final review-impl below is clean for the current committed head.
• Both anchors cite existing analogous code at the same seam.

Every figure below is bound to the head it was measured at, with the command and the counting unit stated, so each can be re-derived rather than trusted.

All figures in this section were measured at head 895582315bcb1b640371c867b9c385d9bf20c700, base acfeaf0786c7f1b39d0226ed965981348a2a5dd3, with DIRTY_TRACKED=0 recorded inside the gate log itself rather than asserted afterwards — except the probe row, whose different binding is stated in that row.

• npx vitest run (cwd client, no path filter; unit = one executed it()) — 2567 passed / 0 failed / 12 todo (2579) across 286 files passed / 3 skipped (289); exit 0
• An intermittent failure, reported with its rate rather than re-rolled until green. Across the last five full-suite runs (two heads), three failed with exactly one test failing and 2566 passing. The two whose output I captured were both src/hooks/__tests__/useCardImage.test.tsx > uses imported source printing art by default when no art chain is configured, failing as Error: Test timed out in 5000ms — an elapsed-time failure, not an assertion. The third was a gate-script run whose log filters the test name, so I am not claiming it was the same test; I am saying I do not know. That file passes 3/3 in isolation, is not among this PR's 17 paths, and this PR touches nothing under hooks/ or any image path — so I believe it is a pre-existing load-sensitive flake on a busy box, not a regression here. I have not proven that on the base commit, so it is stated as a belief with its evidence, not as a finding. Recorded because quoting whichever run happened to be green is the same defect as quoting a stale SHA — and because if CI trips on this, the reviewer should already know it exists.
• npx tsc -b --noEmit --force (unit = exit code; -b, not -p, which is vacuous on this solution config) — exit 0
• must-fail control for the above, re-run at this head (mutate parseAmount's return type number | null → string | null, one unique anchor) — exit 2, 9 error TS (5× TS2322, 2× TS2365, 2× TS2367), landing in AmountInput.tsx and all three adopting prompts, which is the evidence the gate reaches every call site and not merely the definition; one error lands on aria-valuenow, so the new value semantics are type-checked too. Reverted by byte-copy and touch, md5-verified back to pristine bcdaa03e3f5c1839bc1c981617373e08. (The md5 recorded here in an earlier revision was e11b7eb5…, measured before px-0 was removed from the file — re-run rather than relabelled, because carrying a control's figure across a change to the file it mutates is exactly the false attribution this PR keeps correcting.)
• npx eslint . (unit = one error / one warning) — exit 0; 28 problems, 0 errors, 28 warnings. Two of those warnings are this PR's (react-refresh/only-export-components, from AmountInput.tsx exporting parseAmount beside the component); the other 26 pre-date the branch. Disclosed rather than reported as a bare "lint clean".
• i18n parity (unit = one locale's mana keyset; json.load + set equality per locale) — 7 locales, 48 mana keys, identical across all 7
• 22/22 discriminating probes on the first two commits' tests — each with mutation, literal command, ran-count, the failing test named, and a named green paired positive. Every arm reverted by byte-copy and touch, md5-verified back to pristine. These are bound to da694dd44 and I am NOT carrying them forward as current evidence. An earlier revision of this body argued they still held because only acfeaf078 sat between that head and this one, intersecting none of the 17 paths. That argument is dead, and my first attempt at retracting it also understated the drift — the review that caught the stale claim caught the correction too. Measured: git rev-list --count da694dd44..895582315 = 10 (seven new PR commits, plus acfeaf078, plus two originals the rebase rewrote), and git diff --name-only da694dd44..895582315 returns 17 paths — 16 of this PR's 17, every file except test/factories/gameStateFactory.ts. AmountInput.tsx alone is +63/−11 over that range and the content is not cosmetic: role="spinbutton", aria-valuenow/min/max, the role="status" live region and the 44px sizing all landed after the probes ran. The four probe-carrying test files changed by +429 lines. "Measurably inert" was true of the px-0 removal and of nothing else, and I should not have let it characterise the whole delta. Re-running them exactly is not possible — they were executed interactively and no harness was kept, which is my error and is why the rule I wrote down at the time ("a probe row is not evidence until it has been RUN") now has to be applied against my own earlier row. What is current-head evidence instead: the mutant table below (six rows, each re-measured at a recent head, each naming the row it reds) and the tsc must-fail control above, which was re-run at this exact head. If you want the original 22 reconstructed and re-run before merge, say so — it is a real cost, so I am not spending it unasked, but I am also not claiming coverage I did not re-measure.

Engine legs (cargo clippy --workspace, cargo test -p phase-engine) are not run and are not applicable: this PR touches no Rust file. Stated rather than silently omitted.

Gate A

Gate A PASS head=895582315bcb1b640371c867b9c385d9bf20c700 base=acfeaf0786c7f1b39d0226ed965981348a2a5dd3

Run as ./scripts/check-parser-combinators.sh acfeaf0786c7f1b39d0226ed965981348a2a5dd3 (Gate G also PASS in the same output). Why this PASS is honest rather than impressive: the range is non-empty — 9 commits, 17 files — but contains 0 Rust files, so the parser-combinator gate has nothing to bite on here. It is a real run over a real range, not a vacuous empty-range PASS, and equally it is not evidence that parser scrutiny occurred.

Anchored on

• client/src/components/mana/ManaPaymentUI.tsx:41 — the same prompt seam this PR's three components follow, and untouched by this PR, so it can be checked independently of the diff: useTranslation("game") + useGameStore((s) => s.waitingFor) + useGameStore((s) => s.dispatch), with the prompt discriminated on waitingFor?.type. The new box is factored out along that existing seam rather than invented beside it.
• client/src/test/factories/gameStateFactory.ts:270 — the pre-existing ChooseXValueWaitingForFactory (present in the base at :269, so it is genuinely prior art and not something this PR introduced) that the new PayAmountChoiceWaitingForFactory at :288 mirrors exactly, including the paired build* helper and the builder methods.

Final review-impl

Final review-impl head=895582315bcb1b640371c867b9c385d9bf20c700 — committed diff PASS; 7 findings, every one of them in this document.

Read-only, and it took eight rounds — 4 findings, then 2, 2, 1, 4, 4, 6, and finally 7. I am not recording that as a bare PASS, because the last round was not one: it found the committed diff clean (3 files, 26 lines, 0 non-comment lines) and returned 7 findings entirely against this PR description — a stale evidence claim, a corrected sentence that had been fixed in the code and not here, and five figures quoted without their head. Those are fixed above. The pattern worth naming is that once the code settled, every remaining defect was in the prose about the code, including in the paragraph retracting an earlier bad claim — the retraction understated the drift it was retracting. Rounds 2 and 4 each found a defect in the previous round's fix, which is the honest reason for the count: the accessibility fix in round 1 made the refusal audible and left the value silent, and my write-up of the round-1 measurement named the wrong mechanism. Both are described above rather than folded in quietly.

Claimed parse impact

None. No parser code is touched.

Scope Expansion

None.

Validation Failures

None.

CI Failures

None.

---

Deliberate behaviour flips, disclosed

ChooseXValueUI changes in three ways, each with its test updated deliberately rather than adjusted to fit:

1. Upper-bound clamp — an over-max entry is now refused and announced, not silently clamped.
2. Lower-bound blur reset — a below-min entry resets on blur instead of committing.
3. Stepper gating — the steppers now stay live while the entry is invalid, so a player can step back into range instead of being stranded.

What the final review-impl changed, disclosed

The six items below came out of the first five rounds — 4 findings, then 2, then 2, then 1, then a closing verification; the series went on to seven rounds in total, and the later ones are described where they land. All six are fixed in the third commit, so the diff you are reading is not the one that was reviewed first. Rounds 2 and 4 both found defects in the fixes from the round before, which is the main reason this went past a single pass:

1. Accessibility, the refusal. The out-of-range copy was associated via aria-describedby but never announced: those attributes are resolved by a screen reader when focus arrives at the control, and here both flip while focus is already inside the box — so a blind player got no feedback and would have to tab away and back to discover the entry was refused. The node is now a permanently-mounted role="status" aria-live="polite" region whose text mutates (mounting a node into a live region is equally unreliable). This reuses the idiom already present in 14 files of this client rather than inventing one — 14 is the count at the base commit; the grep returns 16 at head because it now includes this PR's own two files, which would be circular to cite.
2. Accessibility, the value — a regression this PR introduced and round 2 caught. The box replaced <input type="range"> (role slider) and <input type="number"> (role spinbutton), both of which expose their value natively, with a bare type="text". The accepted amount therefore became inaudible: +/− and the arrow keys mutated a value nothing announced, since the live region carries only the refusal. A note in the file had rejected role="spinbutton" for having "zero in-repo precedent" and forcing aria-value upkeep — both premises were false (ManaCurve.tsx:59 already uses aria-valuenow, and the replaced controls carried these roles natively). The box now carries role="spinbutton" with aria-valuenow/valuemin/valuemax; it already implemented the entire spinbutton keyboard contract, so the role is descriptive rather than decorative. aria-valuenow reports the validated amount and is absent while out of range, so it never contradicts aria-valuemax.
3. The button labelled a value the player never typed. An over-max Assist entry read "Pay nothing" — the opposite of the pending intent — and 1001 tokens read "Create 0 tokens", while ChooseX previewed min's mana cost. That is this branch's own defect displaced from the dispatch to the display. Labels now name the action without a value (one new mana.confirmAmount key across all 7 locales; mana.pay was rejected as a reuse because the loop-collapse button says "Create", not "Pay") and the cost preview is suppressed while there is no chosen X.
4. ChooseXValueUI's reset effect omitted max, so re-entry that narrows max while leaving min unchanged left defaultValue identical, never fired the effect, and stranded an out-of-range entry with no way to self-heal. Both sibling prompts already keyed on the full window.
5. Four negative dispatch assertions were dominated by their toBeDisabled() sibling — see the next section, because the fix is not the one that was suggested.
6. Deleting the slider silently invalidated a comment in another file. DialogHost's Frontend: mana payment X-value input box unresponsive until clicking elsewhere on board #2427 residual-transform guard cited ChooseXValueUI's <input type="range"> as its live example. That slider no longer exists, so a maintainer checking the comment would find nothing, conclude the guard was obsolete, and delete it — regressing Frontend: mana payment X-value input box unresponsive until clicking elsewhere on board #2427 for the ± steppers that this branch's own retargeted mount-integration test exists to protect. The comment is repointed at the surviving subjects. This is the only component file outside components/mana/ that the PR touches (the others are test/factories/gameStateFactory.ts and the 7 locale JSONs), and the change is comment-only.
7. An overstated conformance claim, in the sentence justifying the new role. I had written that the box implements the "full spinbutton keyboard contract"; the APG pattern also lists Home/End (jump to min/max) as non-optional, and only Enter/Arrow keys are handled. The claim is narrowed rather than the keys added: Home/End carry load-bearing caret semantics in a real editable text field, and native <input type="number"> — whose implicit role is already spinbutton — does not remap them either. Left as-is deliberately, and now said so.

One reviewer remedy measured and rejected, stated plainly

For finding 4 the review suggested clicking the disabled button so expect(dispatch).not.toHaveBeenCalled() would gain signal. I measured that remedy and it does not work. Deleting handleCommit's if (amount === null) return; reds only T6b — the Enter row — while T2/T3/T4/T5 stay green with the click, because React does not dispatch onClick to a disabled button at all. Enter bypasses disabled and is the only route on which that guard is observable, which this branch's own earlier commit message already said.

So the rows are labelled DOMINATED per the convention this test file already uses, the discriminating row is named in each label, and the pre-existing inert click in T5 was removed rather than replicated three more times — leaving it would imply coverage the row does not have. Reporting this because a silently-different fix reads as a silently-ignored finding.

And my first write-up of that measurement was itself wrong, which round 2 caught: I attributed the suppression to jsdom, but this suite runs happy-dom (client/vitest.config.ts:64) — jsdom is never loaded. The behaviour and the conclusion were right, the mechanism was not; it is React-level and therefore invariant under a change of test environment, which is the opposite of what my wording implied. Corrected in all three comments and in the commit message. Recording it because the finding is exactly the failure mode this PR's review kept surfacing: an authority named for something it does not govern.

Each new test is proven discriminating by an isolated mutant that reds it, by name, with no collateral failure:

Mutant	Reds
restore the conditional mount of the message node	A11Y/live-region: the validation copy lives in a polite region that pre-exists it
delete aria-valuenow	A11Y/value: the accepted amount is exposed, and withheld while out of range
source aria-valuenow from the raw digits instead of the validated amount	A11Y/value (the withheld half — a distinct mutant, so both halves are pinned independently)
drop max from the reset dep array	resets an entry stranded above a narrowed max when min is unchanged
restore amount ?? min in the cost-preview memo	hides the pending cost preview while the entry is invalid
re-guard Enter inside the box (if (amount !== null) onSubmit())	enter: Enter calls onSubmit, and calls it EVEN when the entry is out of range

The max row holds min constant deliberately: the pre-existing re-entry test narrows min as well, so defaultValue changes and the effect fires either way — it cannot discriminate the missing dependency. Every arm was reverted by byte-copy and touch, md5-verified back to pristine.

CodeRabbit round, and one fix deliberately left incomplete

Two Major findings, both valid, both fixed in the fourth commit.

Touch targets — and my first fix for it was wrong, by 2px. The controls were 36px against this repo's 44pt rule (.coderabbit.yaml, client/src/index.css). I first tried to keep the 36px visual box and widen only the hit area with the ::before idiom from board/ManualManaToggle. That measured 42px, not the 44px my own comment claimed: gameButtonClass includes border, and an absolutely positioned pseudo-element resolves against its ancestor's padding box (36 − 2×1 = 34), so -inset-1 gives 34 + 8. The precedent did not transfer because ManualManaToggle uses ring-1, which adds no layout border.

Rather than retune the inset, the pseudo-element is deleted and the controls are plainly h-11 (44px). A size that must be derived through two CSS rules to be checked is a size that will silently regress — this one already had, within a single commit. The numeric box is 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 trick anyway — <input> is a replaced element and renders no pseudo-elements). Correcting a figure I gave earlier: h-9 w-9 appears 16 times in this client, not 33 — I had conflated three grep patterns. That 16 is bound to 35effbdc8; at this head it is 12 (14 at the base, +2 added and then −4 removed by the h-11 fix). Quoting a count without its head is the same defect as quoting a gate figure without one.

Prompt-identity resets — fixed as far as the display layer legitimately can, and no further. The three reset effects keyed only on prompt type and bounds, so a successor prompt with the same window left the entry holding the previous decision. This is the same defect class as the max-dependency bug above: a reset keyed on an incomplete identity. Each effect now also keys on the identity its wire type carries — source_id, caster, and the pending cast's object_id. Each regression test holds the window constant so the identity field is the only changing dependency, and each is proven by a mutant removing exactly that dependency, redding its own row by name.

The identity had to be widened once more, and my first residual claim was too broad. Keying on the source alone missed the axis the engine actually varies. The case that shows it is the LoopCollapse arm in game/turns.rs — the prompt this component's own tests model — 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, so only max and the collapse axis derive from state: two controllers with equal counts on the same axis give successive prompts differing in player alone. (I first justified this with an equal-life case on effects/pay.rs. That does not hold: pay.rs does drive PlayerFilter::All and its test does show a constant source_id with player 0 then 1, but max 2 → 3 and accumulated 0 → 2 move as well, so [min, max] alone would already have fired — and it is the mana-X arm, not the life arm I described. The test I cited did not show what I said it showed.) AssistPayment likewise carries chosen, the seat actually asked to pay, so my earlier comment claiming caster was "the only identity this wire type carries" was simply false. Both dep arrays now include the seat.

Both seat rows use the CR 723.5 + CR 723.3 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 therefore what makes the bug reachable at all. 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.

I first cited CR 723.1a here and in both test comments. That is the wrong subrule: 723.1a is "multiple player-controlling effects that affect the same player overwrite each other," an overwrite rule that says nothing about who makes the decisions. The rules that govern the claim are CR 723.5 ("while controlling another player, a player makes all choices and decisions the controlled player is allowed to make") and CR 723.3 ("a player who's being controlled during their turn is still the active player") — the latter being why the active player moves while the controller does not. Both were grepped from docs/MagicCompRules.txt before rewriting, and CR 723.5 is already what this repo cites at this exact seam in game/autoPass.ts, adapter/p2p-adapter.ts and engine/src/game/turns.rs. This is the sixth citation-that-does-not-reach-its-claim in this workstream and the second I introduced while fixing one; the recurring shape is that a citation gets checked for existence and never for reach.

What genuinely remains is narrower still, and I have now had to narrow it three times — worth saying, because each time the claim was broader than the measurement behind it, and each narrowing came from someone counting the fields I had not. The PayAmountChoice variant has seven: player, resource, min, max, accumulated, source_id, pending_mana_ability (types/game_state.rs, mirrored in full at adapter/types.ts). Four are in the dep array. accumulated and resource are not, because I could not demonstrate a reachable path needing them — unlike the seat axis, which turns.rs's LoopCollapse arm proves by construction. pending_mana_ability is not, and cannot be: it crosses the wire as unknown, so the display layer has nothing to compare. So the honest statement is the one measured off the dep array rather than off the variant: the effect keys four of the seven (player, min, max, source_id), so any successor matching those four is unreset — including one differing only in accumulated or resource, and finish_pay_amount_choice does stamp accumulated on a chained successor. No such path is demonstrated, which is why those two are not deps; but "identical in all seven" was the wrong bound to quote, because it describes a stricter successor than the code actually requires. Stating the weaker true bound rather than the stronger convenient one. Closing even that would need an engine-emitted prompt identity; deriving one client-side is the "frontend infers game state" mistake CLAUDE.md prohibits. Happy to do that engine change on request rather than smuggle it into a client-only PR.

The reachability evidence was also repointed. The comment in PayAmountChoiceUI.tsx originally leaned on an equal-life case on the pay arm, which I never demonstrated: pay.rs's own test does show consecutive prompts with constant source_id and player 0 → 1, but max and accumulated move there too, so [min, max] alone would already have fired. The case that actually carries the claim is the LoopCollapse arm in game/turns.rs — the very prompt this component's tests model — which writes min: 0, accumulated: 0, source_id: ObjectId(0) and pending_mana_ability: None as literals, constant by construction rather than conditional on board state; only max and the axis vary per controller. Two controllers with equal counts on the same axis therefore differ in player alone. Recording the swap because "provably" was doing work in the old comment that the cited test did not support.

CodeRabbit's second round — one finding real, one refuted by mutation

The review also filed a Nitpick naming two behaviors in AmountInput as uncovered. One was; one was not, and the difference was settled by mutating rather than by reading.

Real: onSubmit was passed as vi.fn() by every row and never asserted. The caller suites (T6b, AP/enter) do exercise Enter, but only through "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 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. The new row pins it: re-adding the forbidden guard inside the box reds exactly one row, this one — no collateral, which also confirms nothing else was covering it.

Refuted: the min > 0 hint branch is already covered, twice over. Collapsing the ternary to the max-only string reds ChooseXValueUI's existing min 1 / max 10; collapsing it to the two-sided string reds three rows, because each asserts the hint's full text rather than a substring. A candidate row asserting both legs does red alongside them — what it fails to grow is the two mutants' set of detections, not their failure count, which is exactly what "dominated" means. So it is left out and the measurement is recorded in the file 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. The two mutants are re-runnable from that comment.

Predecessor context

This is the first of two PRs from one plan. The second (scheduled_collapse → the ∞→N badge affordance) depends on #7002 and will follow it; the split is at a measured dependency boundary, not a size boundary — the badge half has no rendered ∞ badge to attach to until #7002 lands. The two halves were verified non-interfering in both directions before splitting: a plan-text scan for 18 cross-half identifiers returned 0 with a positive control proving the instrument returns non-zero.

Not done, disclosed

No in-browser playtest of a live 4p game. Acceptance here is component- and suite-level.

Summary by CodeRabbit

• New Features
  • Added a consistent amount-entry control for mana payments and choices.
  • Enter numeric values directly, use Enter to confirm, or adjust amounts with stepper buttons and arrow keys.
  • Inputs enforce available minimum and maximum values, clearly indicate invalid entries, and prevent invalid submissions.
  • Valid amounts now drive accurate cost previews and confirmation actions.
• Accessibility
  • Added accessible labels, range descriptions, status messaging, and spinbutton support.
• Localization
  • Added updated amount-entry and validation messages across supported languages.

[coderabbitai]

📝 Walkthrough

Walkthrough

Changes

The PR adds a shared bounded AmountInput control. Mana payment interfaces now use raw input validation, keyboard stepping, accessible feedback, and localized labels. Tests cover valid, invalid, bounded, keyboard, recovery, and submission states.

Mana amount entry

Layer / File(s)	Summary
Shared amount control and validation client/src/components/mana/AmountInput.tsx, client/src/components/mana/__tests__/AmountInput.test.tsx	Adds digit-only parsing, inclusive bounds checks, controlled input, steppers, Enter submission, keyboard navigation, spinbutton metadata, validation messaging, and recovery behavior.
Mana payment integrations client/src/components/mana/AssistPaymentUI.tsx, client/src/components/mana/ChooseXValueUI.tsx, client/src/components/mana/PayAmountChoiceUI.tsx, client/src/components/mana/__tests__/*	Replaces range sliders with AmountInput. Invalid values disable confirmation and do not dispatch. Valid values, including zero where allowed, dispatch with engine-provided bounds. Tests cover payment, X selection, loop collapse, seat gating, and DialogHost interactions.
Localization and test-state wiring client/src/i18n/locales/*/game.json, client/src/test/factories/gameStateFactory.ts, client/src/components/modal/DialogHost.tsx	Adds localized amount validation and control labels, adds PayAmountChoice waiting-state factories, and updates the DialogHost pointer hit-testing comment.

Estimated code review effort: 4 (Complex) | ~45 minutes

Sequence Diagram(s)

sequenceDiagram
  participant Player
  participant AmountInput
  participant ManaPaymentUI
  participant GameState
  Player->>AmountInput: Enter or adjust amount
  AmountInput->>ManaPaymentUI: Report raw value
  ManaPaymentUI->>AmountInput: Validate against min and max
  Player->>ManaPaymentUI: Confirm valid amount
  ManaPaymentUI->>GameState: Dispatch validated payment

Loading

Suggested labels: quality

Suggested reviewers: matthewevans

🚥 Pre-merge checks | ✅ 4 | ❌ 1

❌ Failed checks (1 warning)

Check name	Status	Explanation	Resolution
Docstring Coverage	⚠️ Warning	Docstring coverage is 50.00% which is insufficient. The required threshold is 80.00%.	Write docstrings for the functions missing them to satisfy the coverage threshold.

✅ Passed checks (4 passed)

Check name	Status	Explanation
Linked Issues check	✅ Passed	Check skipped because no linked issues were found for this pull request.
Out of Scope Changes check	✅ Passed	Check skipped because no linked issues were found for this pull request.
Description Check	✅ Passed	Check skipped - CodeRabbit’s high-level summary is enabled.
Title check	✅ Passed	The title clearly and concisely summarizes the shared sanitized numeric input added for all three amount prompts.

✨ Finishing Touches

🧪 Generate unit tests (beta)

• Create PR with unit tests

---

Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out.

❤️ Share

• X
• Mastodon
• Reddit
• LinkedIn

_{Comment @coderabbitai help to get the list of available commands.}

[coderabbitai]

Actionable comments posted: 2

🧹 Nitpick comments (1)

client/src/components/mana/__tests__/AmountInput.test.tsx (1)

205-237: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick win

Add a test for Enter submission and for the min > 0 hint branch.

Two behaviors in AmountInput have no coverage here.

1. Enter submission. onSubmit is passed as vi.fn() in every test but never asserted. The Enter branch is the submission entry point, and AmountInput deliberately does not re-guard the amount there. Removing the Enter handler leaves this suite green.
2. The min > 0 hint. Every test uses min={0}, so only mana.maxOnly renders. The mana.minMax branch is unreached.

💚 Proposed tests

+  it("submits on Enter, leaving the null guard to the caller", () => {
+    const onSubmit = vi.fn();
+    render(
+      <AmountInput
+        raw="9"
+        onRawChange={vi.fn()}
+        min={0}
+        max={5}
+        onSubmit={onSubmit}
+        labels={LABELS}
+      />,
+    );
+
+    fireEvent.keyDown(screen.getByLabelText("Enter amount"), { key: "Enter" });
+    expect(onSubmit).toHaveBeenCalledTimes(1);
+  });
+
+  it("announces both bounds when min > 0", () => {
+    render(
+      <AmountInput
+        raw="5"
+        onRawChange={vi.fn()}
+        min={2}
+        max={5}
+        onSubmit={vi.fn()}
+        labels={LABELS}
+      />,
+    );
+
+    const box = screen.getByLabelText("Enter amount");
+    const hintId = box.getAttribute("aria-describedby") ?? "";
+    expect(document.getElementById(hintId)?.textContent).toBe("min 2 / max 5");
+  });

🤖 Prompt for AI Agents

Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.

In `@client/src/components/mana/__tests__/AmountInput.test.tsx` around lines 205 -
237, Add coverage in the AmountInput tests for the Enter key path by providing a
named onSubmit mock, firing Enter on the labelled input, and asserting
submission is invoked. Add a case with min greater than zero and max set so the
mana.minMax hint branch is rendered and asserted, while preserving the existing
min=0 coverage for mana.maxOnly.

🤖 Prompt for all review comments with AI agents

Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.

Inline comments:
In `@client/src/components/mana/AmountInput.tsx`:
- Around line 93-106: Increase the touch target for both stepper buttons in
AmountInput while preserving their 36px visual box: update the decrease button
near step(-1) and the corresponding increase button near step(1) to provide at
least 44pt of clickable area using padding or an equivalent hit-area technique.
Keep the compact visual styling unchanged.

In `@client/src/components/mana/AssistPaymentUI.tsx`:
- Around line 27-29: Reset input when the engine replaces a prompt with the same
bounds by adding each prompt’s stable identity to the relevant useEffect
dependencies: update AssistPaymentUI.tsx lines 27-29, ChooseXValueUI.tsx lines
57-64, and PayAmountChoiceUI.tsx lines 23-25. Add regression tests replacing
each prompt directly with a same-bounds successor, covering back-to-back prompts
and confirming raw input resets.

---

Nitpick comments:
In `@client/src/components/mana/__tests__/AmountInput.test.tsx`:
- Around line 205-237: Add coverage in the AmountInput tests for the Enter key
path by providing a named onSubmit mock, firing Enter on the labelled input, and
asserting submission is invoked. Add a case with min greater than zero and max
set so the mana.minMax hint branch is rendered and asserted, while preserving
the existing min=0 coverage for mana.maxOnly.

🪄 Autofix

Fix all unresolved CodeRabbit comments on this PR:

• Push a commit to this branch (recommended)
• Create a new PR with the fixes

---

ℹ️ Review info

⚙️ Run configuration

Configuration used: Path: .coderabbit.yaml

Review profile: CHILL

Plan: Pro Plus

Run ID: 6fdacc8f-b886-4532-8de4-831ce1cceb6c

📥 Commits

Reviewing files that changed from the base of the PR and between acfeaf0 and 501d94c.

📒 Files selected for processing (17)

• client/src/components/mana/AmountInput.tsx
• client/src/components/mana/AssistPaymentUI.tsx
• client/src/components/mana/ChooseXValueUI.tsx
• client/src/components/mana/PayAmountChoiceUI.tsx
• client/src/components/mana/__tests__/AmountInput.test.tsx
• client/src/components/mana/__tests__/AssistPaymentUI.test.tsx
• client/src/components/mana/__tests__/ChooseXValueUI.test.tsx
• client/src/components/mana/__tests__/PayAmountChoiceUI.test.tsx
• client/src/components/modal/DialogHost.tsx
• client/src/i18n/locales/de/game.json
• client/src/i18n/locales/en/game.json
• client/src/i18n/locales/es/game.json
• client/src/i18n/locales/fr/game.json
• client/src/i18n/locales/it/game.json
• client/src/i18n/locales/pl/game.json
• client/src/i18n/locales/pt/game.json
• client/src/test/factories/gameStateFactory.ts

[matthewevans]

Changes requested — two current-head correctness/accessibility blockers at 501d94c248aecd3a03226d52e5074cecc75eca8e.

🔴 Blockers

[MED] A raw amount can carry into a distinct same-bounds prompt. Evidence: client/src/components/mana/AssistPaymentUI.tsx:27-29, ChooseXValueUI.tsx:57-64, and PayAmountChoiceUI.tsx:23-25 reset only on prompt kind/window; client/src/stores/gameStore.ts:451-455 replaces waitingFor whenever an accepted engine snapshot arrives. Why it matters: two back-to-back decisions with identical bounds can submit the first prompt’s value for the second decision. Suggested fix: key each reset to the engine prompt’s stable identity as well as its bounds, and add a regression for a direct same-bounds successor for all three prompt variants.

[MED] The recovery steppers are below the project’s coarse-pointer target size. Evidence: client/src/components/mana/AmountInput.tsx:93-103,145-155 forces each button to h-9 w-9 (36px), and client/src/components/ui/buttonStyles.ts:19-26 supplies only min-h-9; client/src/index.css:243-247 documents the 44pt rule. Why it matters: when typed input is invalid, these are the only non-typing recovery controls and are too small for reliable touch interaction. Suggested fix: provide at least a 44px hit target for both steppers while retaining the compact visual treatment if desired.

🟡 Non-blocking

The current CodeRabbit review also correctly notes that AmountInput still lacks direct coverage of Enter submission and the min > 0 hint branch. Please add these while extending the successor-prompt regressions if practical; they are not the reason this review blocks.

Recommendation: address the two blockers on a new head, then request re-review.

[lgray]

🤖 AI text below 🤖

@matthewevans — both blockers were already fixed in commits I hadn't pushed when you reviewed, so the head you read (501d94c24) is behind. New head pushed with this comment. They're the same two CodeRabbit raised, so this reply answers both reviews at once, and in both cases I went past the suggested fix for a measured reason.

Your non-blocking note lands half and half: the Enter gap was real and now has a row; the min > 0 hint branch turned out to be already covered twice, so I'm declining that half with the measurement below rather than adding a row that can't fail. Say the word if you'd rather have it anyway.

Touch targets. Fixed, but not the way the proposed diff had it. The suggestion keeps px-0 and the pseudo-element hit-area approach; I tried the ::before idiom first (copying board/ManualManaToggle) and it measured 42px, not 44. gameButtonClass emits border, and an absolutely positioned pseudo-element resolves against its ancestor's padding box — 36 − 2×1 = 34, so -inset-1 yields 42. ManualManaToggle gets away with it because it uses ring-1, which adds no layout border. So the pseudo-element is deleted and the controls are plainly h-11; a size you have to derive through two CSS rules to check is a size that regresses silently, and this one already had, inside a single commit. The px-0 in the suggestion is also inert — compiling the real sheet with this repo's own Tailwind emits .px-2\.5 (from SIZE_CLASSES.xs) after .px-0, so the later rule wins at equal specificity; .w-11 compiles to calc(var(--spacing) * 11) = 44px and preflight sets box-sizing: border-box, so the border box is 44px whatever the padding does. It's removed rather than left reading as load-bearing. (I'm quoting the ordering rather than byte offsets on purpose: the offsets move with the candidate list you compile, so they aren't a stable figure to cite.) The numeric box is h-11 too: it's the primary tap target, and fixing only the steppers left the main control short.

Prompt-identity resets. Fixed in all three files, and the identity had to be wider than the finding described. Keying on source_id alone misses the axis the engine actually varies: the LoopCollapse arm in game/turns.rs 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, so only max and the collapse axis vary — two controllers with equal counts on the same axis produce successive prompts differing in player alone. AssistPayment likewise carries chosen, the seat actually being asked to pay. Both seat fields are now deps, each with a regression test that holds the window constant so the identity field is the only changing dependency, and each proven by a mutant that removes exactly that dep and reds its own row by name.

On the Nitpick — one of the two was real, and the other I'd ask you to drop.

The onSubmit gap was real and is a good catch. It was vi.fn() in every row and never asserted, and the caller suites can't cover it: they assert only 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". The box deliberately does not re-guard — the guard lives once, in each caller's handleCommit — and that contract was written in a comment and checked by nothing. There's now a row for it, and re-adding the forbidden guard reds exactly that one row with no collateral.

The min > 0 hint branch, though, is already covered twice, so I did not add a row for it. Measured both directions: collapsing the ternary to the max-only string reds ChooseXValueUI's existing min 1 / max 10; collapsing it to the two-sided string reds three rows, because each asserts the hint's full text rather than a substring. A candidate row asserting both legs does go red alongside them — but every mutant it catches is already caught, so it grows the failure count without growing the set of detections. That's the definition of a dominated row, and adding one is worse than adding nothing because it reports coverage it isn't supplying. The two mutants are one-liners and I've left them written down in the test file if you want to re-run them.

A few things this branch corrected on its own that are worth surfacing, since they change claims the PR previously made:

• I had cited CR 723.1a for the multiplayer control shape these tests use. Wrong subrule — 723.1a is "multiple player-controlling effects overwrite each other", which says nothing about who makes decisions. It's CR 723.5 (the controller makes all the controlled player's choices) plus CR 723.3 (a controlled player during their turn is still the active player). Corrected in the tests, the commit messages and the PR body; CR 723.5 is already what this repo cites at the same seam in game/autoPass.ts, adapter/p2p-adapter.ts and engine/src/game/turns.rs.
• The justification I'd written for the player dependency cited a effects/pay.rs test that doesn't show what I said it showed — max and accumulated move in that case too, so [min, max] alone would already have fired. Repointed at the turns.rs case above, which is read off literal field initializers.
• The PR body's residual claim was stated as "a successor identical in all seven fields would still be indistinguishable". That's the wrong bound: the dep array keys four of the seven, so a successor differing only in accumulated or resource is also unreset. The body now states the weaker, true bound.
• The body previously argued a set of 22 discriminating probes carried forward from an earlier head because nothing in between touched the relevant paths. That argument is dead — seven commits have since touched four of them — and the probes were run interactively with no harness kept, so they can't be re-run exactly. The body now retracts them as current evidence rather than relabelling the SHA, and names what is current-head evidence instead.

One disclosure on CI: across the last five full-suite runs, three failed with a single test failing — useCardImage.test.tsx, Test timed out in 5000ms, an elapsed-time failure in a file this PR doesn't touch (3/3 green in isolation, zero path intersection with hooks/). I believe it's a pre-existing load-sensitive flake rather than anything here, but I haven't proven that on the base commit, so I'm flagging it in case CI trips on it.

[coderabbitai]

Actionable comments posted: 1

🤖 Prompt for all review comments with AI agents

Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.

Inline comments:
In `@client/src/components/mana/__tests__/AssistPaymentUI.test.tsx`:
- Around line 72-100: Add a per-prompt sequence identifier to the assist-payment
waiting-for data, expose it symmetrically through the engine, WASM, WebSocket,
Tauri, and P2P adapters with round-trip coverage, and include it in
AssistPaymentUI’s reset dependencies. Extend the existing AssistPaymentUI tests
with a successor prompt whose caster, chosen, type, and max_generic are
unchanged but whose identity differs, verifying the raw amount resets.

🪄 Autofix

Fix all unresolved CodeRabbit comments on this PR:

• Push a commit to this branch (recommended)
• Create a new PR with the fixes

---

ℹ️ Review info

⚙️ Run configuration

Configuration used: Path: .coderabbit.yaml

Review profile: CHILL

Plan: Pro Plus

Run ID: 0692c1c7-b148-4e37-bcc4-9d81ffaf52d6

📥 Commits

Reviewing files that changed from the base of the PR and between 501d94c and 8955823.

📒 Files selected for processing (8)

• client/src/components/mana/AmountInput.tsx
• client/src/components/mana/AssistPaymentUI.tsx
• client/src/components/mana/ChooseXValueUI.tsx
• client/src/components/mana/PayAmountChoiceUI.tsx
• client/src/components/mana/__tests__/AmountInput.test.tsx
• client/src/components/mana/__tests__/AssistPaymentUI.test.tsx
• client/src/components/mana/__tests__/ChooseXValueUI.test.tsx
• client/src/components/mana/__tests__/PayAmountChoiceUI.test.tsx

🚧 Files skipped from review as they are similar to previous changes (6)

• client/src/components/mana/AmountInput.tsx
• client/src/components/mana/PayAmountChoiceUI.tsx
• client/src/components/mana/AssistPaymentUI.tsx
• client/src/components/mana/tests/ChooseXValueUI.test.tsx
• client/src/components/mana/tests/PayAmountChoiceUI.test.tsx
• client/src/components/mana/ChooseXValueUI.tsx

[coderabbitai]

🎯 Functional Correctness | 🟠 Major | 🏗️ Heavy lift

Add a per-prompt sequence identifier before shipping this reset flow.

These tests cover only caster and chosen changes. AssistPaymentUI retains raw when a successor prompt has the same type, max, caster, and chosen. The prior valid amount can then dispatch for a different payment decision.

Have the engine emit a prompt identity. Wire it through WASM, WebSocket, Tauri, and P2P. Include it in the reset dependencies. Add a regression test with identical current fields and a new prompt identity.

As per path instructions, “New engine fields exposed to the UI must be wired symmetrically through every adapter (WASM, WebSocket, Tauri, P2P) with a round-trip test.”

Also applies to: 102-133

🤖 Prompt for AI Agents

Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.

In `@client/src/components/mana/__tests__/AssistPaymentUI.test.tsx` around lines
72 - 100, Add a per-prompt sequence identifier to the assist-payment waiting-for
data, expose it symmetrically through the engine, WASM, WebSocket, Tauri, and
P2P adapters with round-trip coverage, and include it in AssistPaymentUI’s reset
dependencies. Extend the existing AssistPaymentUI tests with a successor prompt
whose caster, chosen, type, and max_generic are unchanged but whose identity
differs, verifying the raw amount resets.

Source: Path instructions

[matthewevans]

Approved at current head 895582315bcb1b640371c867b9c385d9bf20c700.

The prior two blockers are addressed: each amount prompt resets on its relevant semantic successor fields, and the recovery controls use explicit 44px sizing. I also checked the remaining sequence-ID suggestion against the engine construction path: WaitingFor::AssistPayment is constructed from the selected assist payer with its existing caster/chosen/max fields; no demonstrated distinct successor retains the relevant decision identity. Adding a new engine-to-adapter identity solely for this UI reset would be unrelated cross-surface scope.

Quality Gate: PASS — this is at the existing prompt-display seam; the current-head tests cover same-window successors and fail under the corresponding dependency mutations; current required CI is green.