fix(client): drop a stale hand reorder instead of surfacing an engine rejection (#5913)

[minion1227]

Closes #5913

Summary

Reordering the hand while a card arrives pops a spurious error flag at the player ("expected 6 ids, got 5"). Nothing they did was wrong — the client sent the engine an order it already had enough information to know was stale.

Root cause

PlayerHand's drag-end builds its ReorderHand order from the hand the gesture was set up against:

const nextOrder = computeReorderedHand(player.hand, objectId, targetSlot, …);
dispatchAction({ type: "ReorderHand", data: { order: nextOrder } });

An in-flight framer-motion drag can fire with a closure holding the pre-change player. If a card is drawn (or discarded/cast) mid-drag, that order no longer names the current hand, and apply_action rejects it — crates/engine/src/game/engine.rs:

if order.len() != player.hand.len() {
    return Err(EngineError::InvalidAction(format!(
        "ReorderHand: expected {} ids, got {}", player.hand.len(), order.len())));
}

The engine is right to reject it; the bug is that the client dispatches it at all and surfaces the rejection.

Fix

• New pure isHandPermutation(order, hand) in handInsertionSlot.ts — same length and same multiset of ids (a same-length order naming a swapped card is caught too, which a length check alone misses).
• Gate the dispatch on it, re-reading the hand at drop time via useGameStore.getState() — immune to the stale closure an active drag can hold.

Why drop the gesture instead of recomputing it: the drop slot was chosen against the old layout, so replaying that index onto a changed hand could place the card somewhere the player never pointed at. Hand order is purely cosmetic (CR 402.3), so discarding a reorder the player can trivially redo is strictly safer than guessing at their intent. The engine's validation remains the authority — it just stops being fed an order the client already knows is invalid.

Anchored on (Gate B — pattern anchoring)

• client/src/components/hand/handInsertionSlot.ts — computeReorderedHand is the existing precedent for "pure, id-generic hand helper that returns null/false rather than letting the caller dispatch a hand-scrambling action"; isHandPermutation is the same shape and guards the same dispatch.
• client/src/components/hand/__tests__/PlayerHand.test.ts — the file is already a pure-helper suite over handInsertionSlot.ts (computeReorderedHand's suppression/no-op cases); the new cases extend it in that style, including the existing "SUPPRESSES the reorder … (data-corruption guard)" invariant this builds on.

Test plan

• pnpm exec vitest run src/components/hand/__tests__/PlayerHand.test.ts

New isHandPermutation cases: pure reordering accepted; unchanged order and empty hands accepted; order computed before a draw rejected (the exact 5-ids-vs-6-hand shape from the report); order computed before a card left rejected; same-length order naming a card no longer in hand rejected (multiset, not set); multiset duplicate handling.

AI-contributor disclosure

Authored with an LLM (Claude). Client-only change; the engine's ReorderHand validation is untouched, so no CR annotations apply beyond the CR 402.3 rationale cited above. Combinator-purity Gate A passes trivially (no parser diff). Local pnpm/Vitest could not be run in the authoring sandbox (network-restricted dependency install); CI runs the frontend suite. Happy to rework anything.

🤖 Generated with Claude Code

[gemini-code-assist]

Code Review

This pull request addresses issue #5913 by introducing a stale reorder guard (isHandPermutation) to prevent dispatching invalid ReorderHand actions when the player's hand changes mid-drag. This is implemented by checking the proposed order against the current hand state retrieved directly from the game store at drop time. Unit tests are added to verify the multiset permutation logic. The review feedback suggests optimizing the drag-end callback dependencies in PlayerHand.tsx by depending on hand instead of the entire player object to prevent unnecessary re-renders when unrelated player properties change.

Important

The consumer version of Gemini Code Assist on GitHub is being sunset. Starting June 18, 2026, new organization installations will be blocked, and all code review activity will officially cease on July 17, 2026.
For more details on the timeline and next steps, please review the Help Documentation.

[gemini-code-assist]

[MEDIUM] Optimize handleDragEnd dependencies to prevent unnecessary HandCard re-renders.

Currently, handleDragEnd depends on the entire player object. This causes the callback to be recreated—and consequently forces all fanned HandCard components to re-render—whenever any player property (such as life total, mana pool, or energy) changes, even if the hand itself remains unchanged.

To optimize this, you can extract hand from player at the top of the component and use it instead of player inside handleDragEnd:

const hand = player?.hand;

And then update handleDragEnd to depend on hand instead of player:

// Inside handleDragEnd:
if (!hand) return false;
const nextOrder = computeReorderedHand(
  hand,
  objectId as ObjectId,
  targetSlot,
  pendingObjectId != null || organizeActive,
);

This avoids unnecessary recreations of the drag-end handler and improves rendering performance during high-frequency player state updates.

[matthewevans]

Deferred by maintainer intake policy — not ignored.

This current head (9f8dd407f472a57033ae0c7f030babe0eb36284c) was triaged as a frontend-only change (client/src/components/hand/PlayerHand.tsx, client/src/components/hand/__tests__/PlayerHand.test.ts, client/src/components/hand/handInsertionSlot.ts) by minion1227. The local frontend-review allowlist does not include this author, so this route does not perform an implementation-diff review or approve the PR.

A maintainer must explicitly take this PR or add a local frontend-review exception before it can receive substantive review. The defer label is a routing marker only, not a verdict on the change.

[minion1227]

Understood on the defer-fe routing — flagging for whenever a maintainer has capacity, no rush.

State of this one:

• CI green across all checks, including Frontend (lint, type-check, test).
• Addressed the one automated review comment in e47862990: handleDragEnd now depends on the hand slice instead of the whole player object, so an unrelated player change (life, mana, counters) no longer rebuilds the callback. Behavior is unchanged — the null guard moves from player to hand, and the drop-time re-read the guard relies on is untouched.
• The engine side is deliberately not modified: apply_action's permutation check stays the authority. This only stops the client feeding it an order it already knows is stale, so the player stops seeing a spurious error flag for a benign mid-drag draw.

One design point worth your eye, since it's a judgement call rather than a mechanical fix: a stale gesture is dropped rather than recomputed against the new hand. The drop slot was chosen against the old layout, so replaying that index could place the card somewhere the player never pointed at — and since hand order is cosmetic (CR 402.3), discarding a reorder they can trivially redo seemed safer than guessing. Happy to switch it to a recompute if you'd rather it always land something.

If a frontend-review exception isn't something you want to hand out, that's completely fine — I'd just appreciate knowing so I can aim future work at engine-side issues instead.

[matthewevans]

Deferred by maintainer intake policy — not ignored.

This refresh applies to the current head (e478629906d00ec4f3b328f7755120626e951658). It remains a frontend-only change, and minion1227 is not in the local frontend-review allowlist. It therefore remains routed for explicit maintainer ownership rather than receiving an implementation-diff review or approval here. The defer-fe label is a routing marker, not a verdict on the change.

[matthewevans]

Blocked: local WASM reorders are absorbed, but the same stale rejection still reaches multiplayer users as an action error.

🔴 Blocker

[MED] The stale-reorder classifier is applied only by WasmAdapter; WebSocket and P2P guest adapters still construct a generic ACTION_REJECTED error for the identical engine rejection. Evidence: client/src/adapter/wasm-adapter.ts:89-90 maps isStaleReorderMessage to STALE_ACTION, while client/src/adapter/ws-adapter.ts:775-784 and client/src/adapter/p2p-adapter.ts:1994-2001 reject their server/host ActionRejected messages as generic errors. Why it matters: dispatchAction suppresses only STALE_ACTION, so a server-hosted player or P2P guest who reorders during a draw still gets the red error from #5913. Suggested fix: move the shared rejection classification to the transport-neutral adapter error boundary and use it for WASM, WebSocket, and P2P action rejections; add one regression at each remote rejection path.

✅ Clean

The current matcher correctly excludes the invalid-actor rejection and covers both engine permutation failures (crates/engine/src/game/engine.rs:3075-3094).

Recommendation: request changes for the shared transport classification and its remote-adapter tests; then re-review.

[minion1227]

You're right, and the reasoning generalises further than my fix did. The rejection reason originates in the engine, so its classification can't depend on which transport carried it — putting that decision inside WasmAdapter was the actual mistake, not just an omission.

Fixed in 9777b8c82 by moving the decision to the shared adapter boundary in types.ts:

• isStaleRejectionMessage — one authority for what counts as benign-stale (actor-authorization + both ReorderHand staleness verdicts).
• actionRejectionError — builds the AdapterError: non-recoverable STALE_ACTION for a stale verdict (the action is void, so retrying is wrong), recoverable ACTION_REJECTED for everything else, leaving existing surface/retry behaviour unchanged.

Wired through all three transports:

Path	Site
WASM	wasm-adapter.ts now uses the shared predicate (no second copy of the matcher)
WebSocket	ws-adapter.ts — ActionRejected + ManaPaymentPreviewRejected
P2P guest	p2p-adapter.ts — action_rejected + mana_payment_preview_rejected

I routed the mana-payment-preview rejections through it too, not just the action ones — they carry the same engine verdicts over the same transports, so leaving them behind would have recreated the split you flagged one layer down.

Regressions, one per remote path (ws-adapter.test.ts, p2p-adapter-multiplayer.test.ts): each asserts a stale ReorderHand yields STALE_ACTION/non-recoverable, paired with a genuine rejection still yielding ACTION_REJECTED/recoverable — so the classifier is proven discriminating rather than blanket-suppressing. That pairing is the part I'd want reviewed hardest: a matcher that swallowed everything would pass a one-sided test.

One disclosure: local pnpm/Vitest can't run in my sandbox (network-restricted dependency install), so these tests are verified by construction against the existing harnesses in each file, not executed locally — CI is the first real run. If either fails I'll fix it promptly.

Ready for re-review whenever you have capacity.

[matthewevans]

Reviewed current head: the requested transport-neutral fix is in place.

The shared actionRejectionError now classifies the engine’s stale hand-order verdict consistently in WASM, WebSocket, and P2P paths, and the added WS/P2P regressions cover the previously missing remote behavior. I found no new code finding on 9777b8c.

Held pending the required frontend and Rust CI currently in progress; no approval or enqueue action yet.

[minion1227]

Heads-up in case you saw the red frontend on 9777b8c: that was my type error, now fixed in 70c3a1769.

actionRejectionError referenced AdapterErrorCode.ACTION_REJECTED, but that code had only ever existed as a bare string literal in the remote adapters — the registry had no such member, so tsc rejected it. I registered it next to STALE_ACTION rather than adding a fourth bare string. Same wire value, so existing comparisons (including server-draft-adapter's three sites, which I left alone as out of scope) are unaffected. Also dropped an unnecessary as never in the new WS regression — ObjectId is number, so the order literal already satisfies GameAction.

Frontend (lint, type-check, test) is green on 70c3a1769 (2m49s), with the WS and P2P regressions running. Rust jobs still in progress.

That failure was a direct consequence of the sandbox limitation I flagged — I can't run tsc/Vitest locally, so a type error I'd normally catch in seconds reached CI instead. Worth weighing when you review: the logic was reviewed clean, but the first real compile happened on your infrastructure.

[matthewevans]

Blocked: the shared stale-rejection classifier still excludes the server-hosted draft game transport.

🔴 Blocker

[MED] ServerDraftAdapter is an EngineAdapter during its game phase and explicitly mirrors the WebSocket flow, but its ActionRejected handler still constructs new AdapterError("ACTION_REJECTED", …, true) at client/src/adapter/server-draft-adapter.ts:666-675. A stale ReorderHand rejection in a server-hosted draft game therefore still surfaces as the red recoverable action error that this PR fixes for ordinary WebSocket and P2P games. The new transport-neutral authority is actionRejectionError in client/src/adapter/types.ts:3063-3066; route the game-phase action and preview rejection handlers through it, and add the corresponding ServerDraftAdapter regression. (The draft-pick DraftActionRejected path is a separate draft protocol concern.)

✅ Clean

70c3a17 correctly registers the pre-existing wire value in AdapterErrorCode and removes the unnecessary test cast. The WebSocket and P2P coverage added on the preceding head exercises the originally reported remote paths.

Recommendation: request changes; use the existing shared classifier for the remaining game transport.

[matthewevans]

Reviewed current head: stale ReorderHand rejections now classify consistently across WASM, WebSocket, P2P, and server-hosted draft game transports, with discriminating regressions for each remote path.

[matthewevans]

Approved on current head 12a54c68f25db0206f32e8314df57cc47ff71e86.

The server-draft mana-preview rejection now shares the established actionRejectionError classifier; stale and ordinary-rejection regressions distinguish the intended behavior. Required Rust and frontend checks are green, and earlier review findings are addressed at this head.

No quality label: this bug fix required multi-round transport hardening.