fix(client): one authority for object activation, and make the attachment fan use it - #6992
Conversation
… deciding sites Four call sites each re-derived "may I activate this object, and what does a click do" from a different premise, so they disagreed. The attachments dialog and the command-zone emblem chip gated on a raw `<x>Actions.length > 0` bucket check with NEITHER a `WaitingFor` gate NOR a seat gate: an opponent's emblem chip was clickable from this viewer's seat, and the attachments dialog stayed interactive at DeclareBlockers. `viewmodel/cardActionChoice.ts` gains the two authorities: - `deriveActivationAffordances(waitingFor, canAct, legalActionsByObject, objects)` returns the `activatableObjectIds` / `manaTappableObjectIds` pair, applying the timing and seat gates once. - `resolveObjectActivation(actions, object, canTapForMana)` returns a typed `ObjectActivation` verdict — `none` / `dispatch` / `choose` — and owns the CR 605.1a mana/non-mana partition and the phase-rs#506 confirmation gate, so no call site inspects an action list again. `GameBoard` now derives the pair from that authority instead of an inline copy and publishes it unchanged on `BoardInteractionContext`. `PermanentCard`, `DialogAttachmentCard` and `CommandZone` consume it; `LibraryPile` is refactor-only. Tests. The authority itself is covered at the unit level (V14/V15/V18/V18b/ V19/V19b-V19e/V20c2/V20d). `GameBoardInteractionWiring.test.tsx` renders the real `<GameBoard/>` and reads both sets back out of the real provider from a child consumer, asserting CONTENTS rather than sizes — a parity test alone could not have caught an unwired provider. The two newly gated call sites get their own timing and seat arms. `BattlefieldZoneOverflow` is the second consumer of the affordance pair and its `activatableObjectIds` read was previously silent: deleting it left all 130 board tests green, so a row that turns it red ships with it. Assisted-by: ClaudeCode:claude-opus-5
…thority THE BUG: with Kilo, Apogee Mind (401) enchanted by Freed from the Real (408), the engine published `legalActionsByObject["408"] = [#0, phase-rs#1]` and the fan was inert — no ring, and clicking Freed did nothing. The fan had exactly one source, an open interaction's projection, so a populated action bucket with no prompt open reached nothing at all. Freed's own `{U}: Untap` was unreachable. `AttachmentFan` gains a second mode. Mode 1 (an engine interaction owns the prompt) is byte-unchanged and still returns early, so exactly one authority can win. Mode 2 runs only when no prompt is open: the ring and the click both come from the shared authority the battlefield already uses, so the fan can never offer what the board would not. The fan is closed BEFORE the chooser opens — it is a `fixed inset-0 z-[120]` backdrop with an `onClick` catcher and `DialogHost` anchors at z-40, so a fan left mounted would paint over the modal it just opened and swallow its clicks. The host card is the fan's anchor and is never one of its picks, even when the engine publishes actions for the host too. `PermanentCard` gains the matching host click (hunk B). An attached Aura/Equipment/Fortification is its own object (CR 301.5 / CR 303.4), but its only in-place affordance is a ~22px peek rendered BELOW the host, under the 44px touch-target floor. When the host itself offers nothing and an attachment does, the click falls through to the full-card chooser. The branch is placed LAST so it can never pre-empt the host's own target / activation / undo intent, and reads the same affordance sets as the host's own ring rather than the raw bucket. Its selection is UNCONDITIONAL, deliberately unlike the plain-click fallback which toggles: a toggle would strand the fan open over a host that just lost its ring and its attachment expansion. Tests. `AttachmentFan.test.tsx` covers mode 2 including a multi-authority hostile fixture where a prompt and a bucket are live at once. `abilityChoiceConsumerWiring.test.tsx` drives the producer and observes the real consumer — `DialogHost`'s overlay — so the store hand-off is measured rather than assumed; both its assertions are `expect.soft` so neither can pre-empt the other, and its third arm exists because the first two are blind to an always-true `selectable`. `PermanentCard.test.tsx` pins the branch ORDER with the actionable attachment held fixed across every arm. Assisted-by: ClaudeCode:claude-opus-5
github-actions
Bot
added
the
needs-maintainer
|
Caution Review failedThe pull request is closed. ℹ️ Recent review info⚙️ Run configurationConfiguration used: Path: .coderabbit.yaml Review profile: CHILL Plan: Pro Plus Run ID: 📒 Files selected for processing (10)
📝 WalkthroughWalkthroughThe PR centralizes activation affordance derivation and action resolution. Board, attachment, HUD, and zone components now use engine-authorized actions to gate activation, dispatch direct actions, or open ability-choice UI. Tests cover timing, permissions, action ordering, grouping, and interaction wiring. ChangesActivation authority and interaction consumers
Estimated code review effort: 4 (Complex) | ~60 minutes Possibly related PRs
Suggested labels: Suggested reviewers: 🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✨ Finishing Touches🧪 Generate unit tests (beta)
Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 3
🧹 Nitpick comments (4)
client/src/viewmodel/cardActionChoice.ts (1)
160-199: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueCollapse the two map iterations and the repeated seat conjunction.
canActForWaitingStateis repeated in all four arms ofplayerCanAct, andlegalActionsByObjectis enumerated twice. One pass over the map with a typed set of enabling states reads closer to the codebase's "one authority, one derivation" shape, and it halves the work for every consumer that calls this per render.♻️ Proposed single-pass derivation
- const playerCanAct = - waitingFor != null - && ( - (waitingFor.type === "Priority" && canActForWaitingState) - || (waitingFor.type === "ManaPayment" && canActForWaitingState) - || (waitingFor.type === "UnlessPayment" && canActForWaitingState) - // CR 118.12a: Disjunctive unless-cost — same input enablement as - // UnlessPayment (player chooses among sub-costs). - || (waitingFor.type === "UnlessPaymentChooseCost" && canActForWaitingState) - ); - - if (waitingFor?.type === "Priority" && canActForWaitingState) { - for (const [idStr, actions] of Object.entries(legalActionsByObject)) { - const objectId = Number(idStr); - const object = objects[objectId]; - if (!object) continue; - if (actions.some((action) => !isManaObjectAction(action, object))) { - activatableObjectIds.add(objectId); - } - } - } - - if (playerCanAct) { - for (const [idStr, actions] of Object.entries(legalActionsByObject)) { - const objectId = Number(idStr); - const object = objects[objectId]; - if (!object) continue; - if (actions.some((action) => isManaObjectAction(action, object))) { - manaTappableObjectIds.add(objectId); - } - } - } + // CR 113.3b: non-mana activations need priority. CR 118.12a: a disjunctive + // unless-cost enables the same mana input as a plain UnlessPayment. + const atPriority = canActForWaitingState && waitingFor?.type === "Priority"; + const manaRingOpen = + canActForWaitingState + && (waitingFor?.type === "Priority" + || waitingFor?.type === "ManaPayment" + || waitingFor?.type === "UnlessPayment" + || waitingFor?.type === "UnlessPaymentChooseCost"); + + if (atPriority || manaRingOpen) { + for (const [idStr, actions] of Object.entries(legalActionsByObject)) { + const objectId = Number(idStr); + const object = objects[objectId]; + if (!object) continue; + for (const action of actions) { + if (isManaObjectAction(action, object)) { + if (manaRingOpen) manaTappableObjectIds.add(objectId); + } else if (atPriority) { + activatableObjectIds.add(objectId); + } + } + } + }🤖 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/viewmodel/cardActionChoice.ts` around lines 160 - 199, Refactor the derivation around playerCanAct and the activatableObjectIds/manaTappableObjectIds updates to evaluate canActForWaitingState once and iterate legalActionsByObject only once. Use a typed set of waiting-state types to determine whether the current state enables actions, then classify each object’s actions into the existing activatable and mana sets while preserving the Priority-only restriction for activatable indicators.client/src/components/board/AttachmentFan.tsx (1)
162-168: 🩺 Stability & Availability | 🔵 Trivial | ⚡ Quick winActivation resolves from two different snapshots. Both click handlers read the action bucket live from
useGameStore.getState()but take thecanTapForManaargument and the entry gate from a render-time value. If an engine snapshot commits between the last render and the click, the resolver partitions a new bucket against an old timing verdict. The mana branch then can drop a legal mana action, or admit one at a state the mana ring no longer covers, andresolveObjectActivationcan returnnoneso the click does nothing.
client/src/components/board/AttachmentFan.tsx#L162-L168: derive the affordances from the samegetState()snapshot used for the bucket, then pass that snapshot'smanaTappableObjectIds.has(id)intoresolveObjectActivationand re-check the gate against it.client/src/components/board/PermanentCard.tsx#L689-L702: replace the context-derivedcanTapForManawith the value derived from the samegetState()snapshot that supplies the bucket, so the entry gate and the partition agree.🤖 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/board/AttachmentFan.tsx` around lines 162 - 168, Use one live game-store snapshot for activation decisions: in AttachmentFan.tsx at lines 162-168, derive affordances from the same getState() result used for legalActionsByObject, re-check the entry gate against that snapshot, and pass its manaTappableObjectIds value to resolveObjectActivation; in PermanentCard.tsx at lines 689-702, replace the context-derived canTapForMana with the value from the same getState() snapshot supplying the action bucket so the gate and partition agree.client/src/components/board/__tests__/PermanentCard.test.tsx (1)
121-127: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winReplace the six positional affordance sets with an options object.
renderPermanentnow takes six defaultedSetparameters. Call sites read asrenderPermanent(new Set(), new Set(), new Set(), new Set(), new Set(), new Set([2]))at line 562. A wrong slot seeds the wrong affordance and the arm still passes, which defeats the mutation-visibility intent stated in the header comments.♻️ Proposed signature
-function renderPermanent( - validTargetObjectIds = new Set<number>(), - selectableSacrificeObjectIds = new Set<number>(), - boardChoiceObjectIds = new Set<number>(), - activatableObjectIds = new Set<number>(), - undoableTapObjectIds = new Set<number>(), - manaTappableObjectIds = new Set<number>(), -) { +function renderPermanent(overrides: { + validTargetObjectIds?: Set<number>; + selectableSacrificeObjectIds?: Set<number>; + boardChoiceObjectIds?: Set<number>; + activatableObjectIds?: Set<number>; + undoableTapObjectIds?: Set<number>; + manaTappableObjectIds?: Set<number>; +} = {}) { + const { + validTargetObjectIds = new Set<number>(), + selectableSacrificeObjectIds = new Set<number>(), + boardChoiceObjectIds = new Set<number>(), + activatableObjectIds = new Set<number>(), + undoableTapObjectIds = new Set<number>(), + manaTappableObjectIds = new Set<number>(), + } = overrides;🤖 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/board/__tests__/PermanentCard.test.tsx` around lines 121 - 127, Update renderPermanent to accept a single options object containing named affordance Set properties instead of six positional parameters, preserving defaults for omitted properties. Migrate every renderPermanent call site, especially the mana-tappable case near the reported call, to use the corresponding property name so arguments cannot seed the wrong affordance.client/src/viewmodel/__tests__/cardActionChoice.test.ts (1)
561-572: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winAdd a seat-gate arm for a payment state.
The seat axis is asserted at
Priorityonly.deriveActivationAffordancesappliescanActForWaitingStateseparately in the mana branch. A change that drops the seat gate from theManaPayment/UnlessPaymentarms leaves every row in this file green, so an opponent's land would ring for this viewer during their mana payment with no failing test.💚 Proposed added arm
it("offers nothing at Priority when the viewer cannot act for the waiting state", () => { const derived = deriveActivationAffordances( PRIORITY, false, legalActionsByObject, objects, ); expect(sorted(derived.activatableObjectIds)).toEqual([]); expect(sorted(derived.manaTappableObjectIds)).toEqual([]); }); + + // The seat gate on the MANA branch, independent of the Priority branch above. + it("offers no mana ring at ManaPayment when the viewer cannot act", () => { + const derived = deriveActivationAffordances( + MANA_PAYMENT, + false, + legalActionsByObject, + objects, + ); + expect(sorted(derived.activatableObjectIds)).toEqual([]); + expect(sorted(derived.manaTappableObjectIds)).toEqual([]); + });As per path instructions: use targeted fixtures that cover timing/seat gates.
🤖 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/viewmodel/__tests__/cardActionChoice.test.ts` around lines 561 - 572, Add a test case similar to the existing "offers nothing at Priority when the viewer cannot act for the waiting state" test, but for a payment state such as ManaPayment or UnlessPayment. Call deriveActivationAffordances with a payment state constant instead of PRIORITY, set canActForWaitingState to false, and verify that both activatableObjectIds and manaTappableObjectIds are empty. This ensures the seat-gate is enforced in the mana branch and prevents opponent lands from being tappable when the viewer cannot act during payment states.Source: Path instructions
🤖 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/board/AttachmentFan.tsx`:
- Line 222: Update the selectable condition in AttachmentFan so interaction
cards are selectable only when the current interaction can submit, alongside the
existing non-host and activation checks. Keep the host exclusion and
non-interaction canActivate behavior unchanged, ensuring visual pick affordances
match handlePick’s canSubmit guard.
In `@client/src/components/hud/DialogAttachmentCard.tsx`:
- Around line 149-153: The outcome handling for resolveObjectActivation uses
partial if-else chains that only handle two of three possible outcomes (dispatch
and choose), leaving the none outcome unhandled and allowing new outcomes to
silently fall through without error. At
client/src/components/hud/DialogAttachmentCard.tsx lines 149-153, replace the
if-else chain with an exhaustive switch statement on verdict.kind that
explicitly handles all three cases: none, dispatch, and choose, and add a never
assertion in a default case to catch any unhandled outcome. Apply the same
exhaustive switch pattern at client/src/components/zone/CommandZone.tsx lines
158-162 to ensure both consumers handle every ObjectActivation outcome variant
explicitly and fail at type-check time if a new outcome is added.
In `@client/src/components/zone/CommandZone.tsx`:
- Around line 146-163: Preserve each grouped emblem’s source identity in
GroupedEmblem instead of resolving activation solely through the
representative.id used by CommandZone. Update handleActivate and the related
activatability checks to inspect every member’s original ID and emblem,
selecting or resolving the available legal action while retaining the existing
dispatch and ability-choice behavior. Add a fixture covering a group where only
a nonrepresentative member has a legal action, and maintain strict CR fidelity
using reusable grouped-emblem logic rather than a card-specific exception.
---
Nitpick comments:
In `@client/src/components/board/__tests__/PermanentCard.test.tsx`:
- Around line 121-127: Update renderPermanent to accept a single options object
containing named affordance Set properties instead of six positional parameters,
preserving defaults for omitted properties. Migrate every renderPermanent call
site, especially the mana-tappable case near the reported call, to use the
corresponding property name so arguments cannot seed the wrong affordance.
In `@client/src/components/board/AttachmentFan.tsx`:
- Around line 162-168: Use one live game-store snapshot for activation
decisions: in AttachmentFan.tsx at lines 162-168, derive affordances from the
same getState() result used for legalActionsByObject, re-check the entry gate
against that snapshot, and pass its manaTappableObjectIds value to
resolveObjectActivation; in PermanentCard.tsx at lines 689-702, replace the
context-derived canTapForMana with the value from the same getState() snapshot
supplying the action bucket so the gate and partition agree.
In `@client/src/viewmodel/__tests__/cardActionChoice.test.ts`:
- Around line 561-572: Add a test case similar to the existing "offers nothing
at Priority when the viewer cannot act for the waiting state" test, but for a
payment state such as ManaPayment or UnlessPayment. Call
deriveActivationAffordances with a payment state constant instead of PRIORITY,
set canActForWaitingState to false, and verify that both activatableObjectIds
and manaTappableObjectIds are empty. This ensures the seat-gate is enforced in
the mana branch and prevents opponent lands from being tappable when the viewer
cannot act during payment states.
In `@client/src/viewmodel/cardActionChoice.ts`:
- Around line 160-199: Refactor the derivation around playerCanAct and the
activatableObjectIds/manaTappableObjectIds updates to evaluate
canActForWaitingState once and iterate legalActionsByObject only once. Use a
typed set of waiting-state types to determine whether the current state enables
actions, then classify each object’s actions into the existing activatable and
mana sets while preserving the Priority-only restriction for activatable
indicators.
🪄 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: f88167c3-affe-444a-8b81-76e0a1aca541
📒 Files selected for processing (15)
client/src/components/board/AttachmentFan.tsxclient/src/components/board/GameBoard.tsxclient/src/components/board/PermanentCard.tsxclient/src/components/board/__tests__/AttachmentFan.test.tsxclient/src/components/board/__tests__/BattlefieldZoneOverflow.test.tsxclient/src/components/board/__tests__/GameBoardInteractionWiring.test.tsxclient/src/components/board/__tests__/PermanentCard.test.tsxclient/src/components/board/__tests__/abilityChoiceConsumerWiring.test.tsxclient/src/components/hud/DialogAttachmentCard.tsxclient/src/components/hud/__tests__/DialogAttachmentCard.test.tsxclient/src/components/zone/CommandZone.tsxclient/src/components/zone/LibraryPile.tsxclient/src/components/zone/__tests__/CommandZone.test.tsxclient/src/viewmodel/__tests__/cardActionChoice.test.tsclient/src/viewmodel/cardActionChoice.ts
| arcOffset={fan.arc(i)} | ||
| zIndex={i} | ||
| selectable={interactionFan !== null && id !== host.id} | ||
| selectable={id !== host.id && (interactionFan !== null || canActivate(id))} |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟡 Minor | ⚡ Quick win
selectable ignores canSubmit, so an unsubmittable interaction still paints a pick affordance.
During an interaction, selectable is true for every non-host card. handlePick then requires viewerInteraction?.canSubmit and returns without an effect when it is false. The card renders the cyan ring, the cursor-pointer class, and the permanent.fanPick badge, and the click does nothing.
Include the submit capability in the affordance so the visual and the action agree.
🐛 Proposed fix
- selectable={id !== host.id && (interactionFan !== null || canActivate(id))}
+ selectable={
+ id !== host.id
+ && (interactionFan !== null
+ ? viewerInteraction?.canSubmit === true
+ : canActivate(id))
+ }📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| selectable={id !== host.id && (interactionFan !== null || canActivate(id))} | |
| selectable={ | |
| id !== host.id | |
| && (interactionFan !== null | |
| ? viewerInteraction?.canSubmit === true | |
| : canActivate(id)) | |
| } |
🤖 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/board/AttachmentFan.tsx` at line 222, Update the
selectable condition in AttachmentFan so interaction cards are selectable only
when the current interaction can submit, alongside the existing non-host and
activation checks. Keep the host exclusion and non-interaction canActivate
behavior unchanged, ensuring visual pick affordances match handlePick’s
canSubmit guard.
matthewevans
left a comment
matthewevans
left a comment
There was a problem hiding this comment.
[MED] Grouped emblem actions can make legal nonrepresentative emblems unreachable. Evidence: client/src/components/game/CommandZone.tsx:28-33 retains only a representative per grouped emblem; :78-88 increments the count without retaining member identities; and :146-161 derives the affordance and dispatches solely from representative.id. legalActionsByObject is keyed by exact object ID, so an action available only for a nonrepresentative member cannot be reached. Why it matters: the command-zone UI can hide a legal action for an emblem that shares a visible group. Suggested fix: preserve the group's member identities, choose a member with an active shared affordance, and resolve/pending-dispatch using that exact ID; add a two-emblem regression where only the nonrepresentative member is live.
…, and an exhaustive verdict Review round on phase-rs#6992. Five fixes, all frontend, all routed through the same two authorities the PR introduced — no new decision site. 1. Exhaustive `switch` + `never` at all FOUR `resolveObjectActivation` consumers (AttachmentFan, PermanentCard, DialogAttachmentCard, CommandZone). Only AttachmentFan's bare `else` errored on a new union variant; the other three compiled clean and silently dropped it. Behaviour is unchanged — `kind: "none"` still does nothing (it is reachable only through the render->click staleness window), and the fan's `close()` stays inside the two acting arms. CLAUDE.md: exhaustive match, no fallback default. 2. `resolveObjectActivation` took only the mana bit of a two-bit gate, so the non-mana partition was merged unconditionally and a cost-payment prompt offered activations the board itself refuses. It now takes the whole `ActivationAffordances` plus the object id and drops the non-mana partition when the activation ring is closed (CR 113.3b), mirroring the existing mana drop. Taking the pair rather than two loose booleans makes "half the gate" unrepresentable at a call site. 3. Sibling sweep: `CommanderCardZone` still gated `canCast`/`canNinjutsu` on the raw bucket with neither a `WaitingFor` nor a seat gate — the exact predicate this PR replaced at `CommandZone.EmblemCard`, in a component `PlayerArea` renders for every seat. Pre-existing, not introduced. 4. Grouped-emblem activation identity (maintainer review, [MED]; CodeRabbit comment 3). `GroupedEmblem` kept a representative plus a tally, so an action the engine published against a non-representative member was unreachable. It now retains every member; the chip acts on the member the shared affordance sets name, and the count badge is that list's length. This changes which id the authority is asked about, not who decides. 5. A `PermanentCard` fixture seeded a mana-only affordance pair while omitting `is_mana_ability` on the abilities — a state the engine cannot emit, since the deriver and the resolver classify through the same `isManaObjectAction`. Fix 2 surfaced it; the flags make the row exercise the mana partition. Two-sided controls, each flipping its own named assertion: the union-variant arms (1 error unfixed / 4 errors fixed / 0 with the real union); `if (true)` and `if (false)` on the activation ring; raw-bucket-restore and constant-false on the commander flags; representative-only and always-last-member on the emblem group. Assisted-by: ClaudeCode:claude-opus-5
|
🤖 AI text below 🤖 @matthewevans — your Path note first, so the evidence trail matches: you cite Confirmed structurally before fixing, not accepted on authority:
The fix is the one you suggested. The regression you asked for, plus its controls —
Two-sided, measured, both arms transcribed:
Full suite at this head: The PR body now also carries the other four fixes from this round, a corrected test-count attribution (+53 tests versus the merge base, measured with two independent instruments — the previous body's "42" was wrong), and two explicitly named follow-ups we did not do: five components each memoizing the same pure affordance call rather than sharing a hook (LOW, no correctness content), and the engine-side reachability of the CR 113.3b gate, which is UNMEASURED because it needs a cargo run this checkout cannot give us right now. The PR stays in draft until CI is green. |
|
🤖 AI text below 🤖 @matthewevans — status update following your review. All five fixes are pushed at Your finding is addressed. Regression added exactly as you specified — two grouped emblems where only the non-representative member is live:
One note on the reference: the path in your review reads CI at this head: Frontend (lint, type-check, test), Rust lint, both Rust test shards, Card data, WASM, Tauri, and Lobby worker all pass. Suite delta is +5 files / +53 tests, attributed by two independent instruments. I don't have permission to request a re-review through the API, so flagging it here instead — this is ready for another look whenever you have time. |
|
Maintainer hold: current-head external review is still running. The grouped-emblem changes-requested finding was addressed on current head |
matthewevans
left a comment
matthewevans
left a comment
There was a problem hiding this comment.
Approved — current-head grouped-emblem activation preserves exact member identity and the command-zone regression covers a live nonrepresentative member.
2 participants
🤖 AI text below 🤖
Summary
An attached permanent's own activated abilities were unreachable. With Kilo, Apogee Mind enchanted by Freed from the Real the engine published
legalActionsByObject["408"] = [#0, #1], and the attachment fan was inert — no ring, and clicking the Aura did nothing. Root cause is that four call sites each re-derived "may I activate this object, and what does a click do" from a different premise; this extracts the two authorities intoviewmodel/cardActionChoice.ts, adopts them at every deciding site, and gives the fan and the host click a path to them.Three of the adopting sites were measurably wrong before the change: the attachments dialog, the command-zone emblem chip, and the command-zone commander chip gated on a raw
<x>Actions.length > 0bucket check with neither aWaitingForgate nor a seat gate, so an opponent's emblem/commander chip was clickable from this viewer's seat and the attachments dialog stayed interactive atDeclareBlockers.Review round (head
5ea5dc100)Five follow-up fixes from the independent
/review-impl(GO, 3 findings, 0 blocking) and from @matthewevans'CHANGES_REQUESTED:switch+neverat all fourresolveObjectActivationconsumers. OnlyAttachmentFan's bareelsefailed to compile against a new union variant;PermanentCard,DialogAttachmentCardandCommandZonecompiled clean and silently dropped it. Today'skind: "none"behaviour is preserved exactly (it is reachable only through the render→click staleness window, where doing nothing is right) — this makes a future variant a compile error, per CLAUDE.md "exhaustivematchwithout wildcard fallbacks".deriveActivationAffordancesemits two bits;resolveObjectActivationtook only the mana one, so the non-mana partition was merged unconditionally. Measured:[P4 ManaPayment] activatableSet=false manaSet=true clickOffered=true verdict=choose[NON-mana, mana]— byte-identical to the[P4 Priority]arm, at a state where the gate had already closed the non-mana ring (CR 113.3b). It now takes the wholeActivationAffordancesplus the object id and drops the non-mana partition when that ring is closed, mirroring the existing mana drop. Taking the pair rather than two loose booleans makes "half the gate" unrepresentable at a call site, which is the actual defect class.CommanderCardZone. It still gatedcanCast/canNinjutsuon the raw bucket with neither aWaitingFornor a seat gate — the exact predicate this PR replaced atCommandZone.EmblemCard, rendered from the samePlayerArea → CommandDocksubtree for every seat. Measured at the un-fixed site:[P3 opponent commander @ opponent priority, viewer=P0] renderedAsButton=true dispatches=1 | sharedAuthorityWouldSay activatable=false, with an own-seat control reportingactivatable=true. Pre-existing rather than introduced by this PR, and swept here because it is the same mechanism.[MED]finding, independently reported by CodeRabbit.GroupedEmblemkept a lonerepresentativeplus acount, andlegalActionsByObjectis keyed by exact object id, so an action the engine published against a non-representative member was unreachable and the chip rendered inert. It now retains every member; the chip acts on the member the shared affordance sets name, and the count badge is that list's length so it cannot drift from the actionable ids. This changes which id the authority is asked about, not who decides — no second decision site, and it is class-general for any group size and any member position. (His evidence citesclient/src/components/game/CommandZone.tsx; that path does not exist —find client/src -name "CommandZone*.tsx"returns onlyclient/src/components/zone/CommandZone.tsx. His line numbers and substance match the real file, so we read it ascomponents/zone/and fixed it there.)PermanentCard.test.tsx > opens the ability picker when a land has multiple mana abilitiesseededmanaTappableObjectIds={40}withactivatableObjectIds={}but omittedis_mana_abilityon both abilities, so the resolver classified them NON-mana — a state the engine cannot emit, since the deriver and the resolver classify through the sameisManaObjectAction. The row only passed because the old code never consulted the activation bit. Adding the flags makes it actually exercise the mana partition.Files changed
client/src/viewmodel/cardActionChoice.ts— the two authorities:deriveActivationAffordances(timing + seat gate → the activatable / mana-tappable id pair) andresolveObjectActivation(typednone/dispatch/chooseverdict; owns the CR 605.1a mana partition, the CR 113.3b non-mana gate, and the Festering Thicket — [[Festering Thicket]] decided it would cycle itself instead of letting me choose whether to play it… #506 confirmation gate)client/src/components/board/GameBoard.tsx— derives the affordance pair from the authority instead of an inline copyclient/src/components/board/AttachmentFan.tsx— mode 2: the fan as a reachability surface for the permanent's own legal actions; mode 1 (engine interaction) unchanged and still returns early; exhaustive verdict handlingclient/src/components/board/PermanentCard.tsx— adopts the authority; adds the last-placed host-click fall-through to the fan; exhaustive verdict handlingclient/src/components/hud/DialogAttachmentCard.tsx— adopts the shared gate (gains the missing timing + seat gates); exhaustive verdict handlingclient/src/components/zone/CommandZone.tsx— same, for the emblem chip; plus grouped-emblem activation identity (members: GameObject[]replacescount+representative)client/src/components/zone/CommanderCardZone.tsx— swept sibling: adoptsderiveActivationAffordancesforcanCast/canNinjutsuclient/src/components/zone/LibraryPile.tsx— refactor onlyclient/src/viewmodel/__tests__/cardActionChoice.test.ts— authority-level rows, incl. the closed-ring rows and a hostile fixture proving membership is keyed by the id passed in, not by set emptinessclient/src/components/board/__tests__/GameBoardInteractionWiring.test.tsx(new) — real<GameBoard/>+ real provider, read back from a child consumerclient/src/components/board/__tests__/AttachmentFan.test.tsx(new) — mode 2, incl. a multi-authority hostile fixtureclient/src/components/board/__tests__/abilityChoiceConsumerWiring.test.tsx(new) — producer → store → realDialogHostconsumerclient/src/components/hud/__tests__/DialogAttachmentCard.test.tsx(new) — timing + seat armsclient/src/components/zone/__tests__/CommandZone.test.tsx(new) — timing + seat + order arms, plus the grouped-emblem identity block (non-representative live; chooser id; representative-live control; all-live single-dispatch control; none-live negative control)client/src/components/zone/__tests__/CommanderCardZone.test.tsx— seat and timing arms for the swept siblingclient/src/components/board/__tests__/PermanentCard.test.tsx— host-click branch-order rows; the mana-ability fixture repair aboveclient/src/components/board/__tests__/BattlefieldZoneOverflow.test.tsx— turns a previously silentactivatableObjectIdsread redTrack
Developer
LLM
Model: claude-opus-5
Tier: Frontier
Thinking: high
Implementation method (required)
Method: not-applicable — frontend-only change; no
crates/engine/game logic is touched (17 files, all underclient/src/).CR references
CR 113.3b(a non-mana activated ability may be activated only when its controller has priority),CR 114,CR 114.1(an emblem is a marker representing its own object),CR 114.4(abilities of emblems function in the command zone),CR 118.12a,CR 301.5,CR 303.4(an attachment is its own object),CR 602.1,CR 605.1a(mana-ability partition). Every number added in the diff was grepped againstdocs/MagicCompRules.txt:0 UNVERIFIED, and the instrument's negative control (702.808) correctly reports absent, so that zero is not vacuous.Verification
All results below are from the pushed head
5ea5dc100(built on38c0dc1aein a detached worktree, since this checkout hosts a concurrent agent's uncommittedcrates/**work).npx tsc -b --noEmit --force— EXIT=0, 0 error lines. Instrument control: with aTS2322injected, the repo's vacuoustsc --noEmit -p tsconfig.jsonstill reportsEXIT=0 errorLines=0while-b --noEmit --forcereportsEXIT=2 errorLines=2.npx eslint .— EXIT=0,✖ 27 problems (0 errors, 27 warnings).eslint .cannot gatereact-hooks/exhaustive-deps(severitywarn), so the gate is the warning set:onlyInBASE=0 onlyInPOST=0, exhaustive-deps 2 on both arms. Instrument positive control for that zero-diff: droppingobjectsfromCommanderCardZone's new memo dep array keepsEXIT=0but moves the set to 28 warnings / 3 exhaustive-deps, with the added line namingCommanderCardZone.tsx.npx vitest run(full configured suite) — EXIT=0,Test Files 285 passed | 3 skipped (288),Tests 2518 passed | 12 todo (2530).it(" and that figure was wrong twice over. Measured on this branch: merge basea970e5548→Test Files 280 passed | 3 skipped (283)/Tests 2465 passed | 12 todo (2477); the previous head38c0dc1ae→288/2520(i.e. +43, not 42); this head →288/2530. The PR's true delta is +5 files and +53 tests, −0 removed (2530 − 2477 = 53). A second, independent instrument agrees: a structural grep census of^\s*(it|test)(\.(only|skip|todo|concurrent))?\(overclient/src/**/*.test.ts{,x}gives2249at the merge base and2302here — delta +53. (The absolute counts differ from vitest's because vitest also counts.each/dynamic rows; the delta is the claim and both instruments give 53.)ObjectActivationvariant, consumers unfixed):EXIT=2, exactly 1 error,AttachmentFan.tsx(176,69) TS2339. ARM B (4th variant + the new switches):EXIT=2, exactly 4 errors,TS2322 … not assignable to type 'never'atAttachmentFan(192,17),PermanentCard(715,17),DialogAttachmentCard(163,17),CommandZone(174,15). ARM C (real 3-variant union + switches):EXIT=0; the synthetic variant is grep-verified removed (0 hits).if (true)for the activation ring) fails the 3 new authority rows, first flipexpected { kind: 'choose', …(1) } to deeply equal { kind: 'none' }; TRIVIALIZE (if (false)) fails 10 rows including every positive control.expected "vi.fn()" to not be called at all, but actually been called 1 times; TRIVIALIZE (both flags constant-false) fails 5 rows.expected false to be trueon the chip's owndata-activatable, andexpected null to deeply equal { objectId: 701, actions: [ …(2) ] }on the chooser id; TRIVIALIZE ("always the last member") fails 5 rows — both controls, the negative control, and the timing + seat rows.act 9, drop renders no badge); the fan's mode-2 drop, never-close, host-invariant drop, early-return drop, never-selectable; and both gates forced constant-true and constant-false.Gate A
Gate A PASS head=5ea5dc100cb04e7eff820f8026e8ef5d2fba8f66 base=4b34e5465eafa94bcd49dfe1a9275968be4300dc
Anchored on
client/src/viewmodel/cardActionChoice.ts:73—resolveSingleActionDispatch, the existing single-authority-for-a-click pattern this extends rather than replacesclient/src/components/board/PermanentCard.tsx:688— the existingisActivatablebranch that already routed through the shared authority; the new sites adopt exactly this shapeFinal review-impl
Final review-impl PASS head=38c0dc1aeb23420604ced0b13431dbd3270f47cd — an independent
/review-implreturned GO, 3 findings, 0 blocking at that head. Head5ea5dc100is the fix round for those findings plus @matthewevans'CHANGES_REQUESTED; no independent review has run at5ea5dc100yet, and the evidence for it is the gate battery and the two-sided controls above.Claimed parse impact
None.
Scope Expansion
The swept sibling
client/src/components/zone/CommanderCardZone.tsx(fix 3) is pre-existing, not introduced by this PR. It is included because it is the same mechanism as theCommandZone.EmblemCardhunk already here, and leaving one copy of a defect the PR is removing everywhere else would be worse than the extra 30 lines.Named follow-ups (deliberately not fixed here)
deriveActivationAffordancescall —GameBoard,AttachmentFan,DialogAttachmentCard,CommandZone.EmblemCard,CommanderCardZone.CommanderCard— rather than sharing auseActivationAffordances()hook, and the last two run per chip / per commander, not once per component. "One authority" in code, several computations at runtime. Rated LOW: it is a shape/perf refactor with no correctness content, and folding it in would widen this diff across five files for no behavioural change.ActivateAbilityneedscrates/**plus a cargo run, and the cargo token belongs to a concurrent agent on this checkout. The fix is fail-closed either way: if the engine never emits that shape, fix 2 is a no-op; if it does, fix 2 stops the UI offering what the board refuses.Validation Failures
The original round's
/review-implwas a self-review; that has since been superseded by an independent/review-implat38c0dc1ae(GO, 3 findings, 0 blocking) and by @matthewevans'CHANGES_REQUESTED. All four findings are fixed at5ea5dc100. The self-review's own two findings (a negative assertion with no reach guard, and an untested arm of the host-click branch on mobile viewports) were fixed before38c0dc1ae.Browser verification was not performed; the evidence is the component-level suite plus the mutant controls above.
Not fixed, because it was measured unreachable — CodeRabbit's first inline comment (
AttachmentFan.tsx:222, Minor): "during an engine interactionselectableignoresviewerInteraction.canSubmit, so an unsubmittable card still paints the pick affordance while the click does nothing." The premise is thatcan_submit: falsecan co-occur with a populatedattachment_fans. It cannot, in the engine as written:derive_viewer_interaction(crates/engine/src/game/interaction.rs:7273-7436) is the sole producer. Repo-widegrep -rn "ViewerInteraction {" crates/returns 10 hits: 7 struct literals, all inside that one function, plus the struct definition and two return-type annotations intests/integration/interaction_contract.rs.VIOLATIONS(can_submit=false AND populated attachment_fans) = 0. Bothcan_submit: falseliterals (:7286,:7302) pair withattachment_fans: BTreeMap::new()(:7289,:7305). The only populated construction (:7425, the field-init shorthand bound at:7373and filled at:7392) setscan_submit: true. The one post-construction mutation,view.attachment_fans.clear()at:7430, is the payload-too-large path and never setscan_submit: false.can_submit: falseliteral makes the same census reportVIOLATIONS = 1. The instrument is not stuck at zero.So the affordance and the click already agree on every reachable state, and CodeRabbit's proposed diff would guard against a state the producer cannot emit. If this is ever tidied, the architecturally correct move is the inverse of that diff — delete the now-redundant
canSubmitre-check insidehandlePick, since a paint/click asymmetry that cannot occur is the smell, not the paint. Recorded as a named follow-up, deliberately not taken while a maintainerCHANGES_REQUESTEDis open: adding an unrequested cosmetic change to this diff at this moment is scope creep.CI Failures
None.
Summary by CodeRabbit
New Features
Bug Fixes