fix: project distinct mana action choices#6639
Conversation
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
📝 WalkthroughWalkthroughThe change adds stable action identities, viewer-specific interaction payloads, live mana restriction projections, updated server/client contracts, protocol version increments, client caching, and localized ability-choice labels. ChangesViewer interaction pipeline
Estimated code review effort: 4 (Complex) | ~60 minutes Sequence Diagram(s)sequenceDiagram
participant Engine
participant PhaseServer
participant WebSocketAdapter
participant GameStore
participant AbilityChoiceModal
Engine->>PhaseServer: derive viewer_interaction and object_action_payloads
PhaseServer->>WebSocketAdapter: send GameStarted or StateUpdate
WebSocketAdapter->>GameStore: cache viewerInteraction and legalActionsByObject
GameStore->>AbilityChoiceModal: provide viewerInteraction and pending actions
AbilityChoiceModal->>AbilityChoiceModal: render projected mana restrictions
Possibly related PRs
Suggested labels: 🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✨ Finishing Touches🧪 Generate unit tests (beta)
Comment |
Parse changes introduced by this PR✓ No card-parse changes detected. |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 7
🧹 Nitpick comments (3)
crates/engine/src/types/interaction.rs (1)
686-687: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick win
action_idisOptionbut neverNone.Every producer (
action_surfaces, both cancel-choice sites) passesSome(interaction_action_id(...)). A non-optional field removes a null branch from every consumer (actionId: InteractionActionId | nullin the generated TS) and matches the "no fallback defaults" rule in CLAUDE.md. If some future surface genuinely has no payload identity, theOptioncan be reintroduced deliberately.🤖 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 `@crates/engine/src/types/interaction.rs` around lines 686 - 687, Change the action_id field in the relevant interaction type from Option<InteractionActionId> to InteractionActionId, then update all producers and consumers—including action_surfaces and both cancel-choice sites—to pass and handle the value directly without Some/None or null fallback branches.crates/engine/src/game/mana_sources.rs (1)
468-491: 🚀 Performance & Scalability | 🔵 Trivial | ⚡ Quick winFull
GameState::clone()on a per-action projection path.This clone exists only to set one scalar (
last_named_choice) before a read-only count query, butlive_mana_output_unitsruns insideproject_action_payload— once per legal action, per viewer, on every snapshot broadcast. A table with several override/aura mana rows multiplies the clone by seats.Prefer threading the chosen color into the resolver (or an explicit
ChoiceValueparameter) so the count can be resolved against&statedirectly.🤖 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 `@crates/engine/src/game/mana_sources.rs` around lines 468 - 491, Update the SingleColor branch in the mana_types resolution flow to avoid cloning GameState solely to set last_named_choice. Thread the selected color into resolve_mana_types_for_ability through an appropriate resolver argument or explicit ChoiceValue parameter, resolve the count against &state, and preserve the existing invalid-color and per-unit color replacement behavior.crates/phase-server/src/main.rs (1)
3779-3843: 🚀 Performance & Scalability | 🔵 Trivial | ⚡ Quick win
derive_viewer_interactionper seat, per AI step, inside the connections lock.This loop now runs a full interaction projection (
opportunity_for_slot+ outbound bounding for every choice) for every seat on every AI follow-up broadcast, while holdingconnections. With a 4-player table and a long AI chain that is a meaningful amount of work under a shared lock, and the surrounding code already takes care to compute filtered states outside locks for the same reason.Consider building the per-seat messages before acquiring
connections, mirroring theai_filteredprecomputation just above, and adding a metric on projection time so regressions here are visible.🤖 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 `@crates/phase-server/src/main.rs` around lines 3779 - 3843, Move the per-seat StateUpdate construction, including derive_viewer_interaction and other payload projections, out of the connections.lock() scope by precomputing messages from ai_filtered before acquiring the lock, then only look up recipients and send inside the lock. Add timing instrumentation/metric for the viewer-interaction projection work so projection regressions are observable.
🤖 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/adapter/types.ts`:
- Around line 3336-3337: Make interactionActionId required on ObjectAction in
client/src/adapter/types.ts (lines 3336-3337). Update
client/src/adapter/__tests__/server-draft-adapter.test.ts (lines 115-117) to
provide and assert a concrete interactionActionId, and update
client/src/network/__tests__/protocol.test.ts (lines 121-129) to include that ID
in legalActionsByObject for a P2P variant and assert it round-trips unchanged.
In `@client/src/i18n/locales/pt/game.json`:
- Around line 1026-1035: Update the Portuguese mana-restriction localization
entries around onlyForSpellWithManaValue, onlyForSpellFromZone, and
onlyForSpecialAction to localize the interpolated comparator, zone, polarity,
and action values instead of rendering raw engine codes. Reuse the existing
localization mechanism and preserve all template parameters and sentence
meanings.
In `@crates/engine/src/game/interaction.rs`:
- Around line 3601-3611: Replace the Debug-based keyword serialization in the
OnlyForSpellWithKeywordKind and OnlyForSpellWithKeywordKindFromZone arms with
the established KeywordKind.keyword_str() engine vocabulary, matching the nearby
tag handling. Preserve the existing zone_code(*zone) conversion and populate
both DTO keyword fields with the stable accessor result.
In `@crates/engine/tests/integration/interaction_contract.rs`:
- Around line 1092-1099: Update the InteractionPresentationSurface::Action
pattern in the choice.surfaces checks to include .. after matching code:
InteractionActionCode::TapLandForMana, allowing the newly added action_id field
while preserving the existing match behavior.
In `@crates/lobby-broker/src/protocol.rs`:
- Line 55: Update the protocol version test assertions to match the new
PROTOCOL_VERSION value: assert PROTOCOL_VERSION is 22 and MIN_SUPPORTED_PROTOCOL
is 21, since it is derived by subtracting one. Update all corresponding
assertions, including the additionally referenced location.
In `@crates/phase-server/src/main.rs`:
- Line 3762: Pass the raw ai_by_object map directly to
StateSnapshotParts.legal_actions_by_object at this call site, matching the other
broadcast-size guard call sites. Remove the object_action_payloads conversion
here; the payload conversion remains handled when constructing the actual send
payload from p_by_object.
In `@crates/server-core/src/protocol.rs`:
- Around line 993-997: Update the round-trip assertions in the affected protocol
tests around the decoded match arms so they explicitly validate
viewer_interaction instead of discarding it with ..; assert at least one
meaningful nested field such as waiting_for_kind.code or availability, plus
interaction_action_id. Also validate the reshaped legal_actions_by_object
Vec<ObjectActionPayload> survives serialization, and ensure these fields are
asserted consistently across every relevant adapter test.
---
Nitpick comments:
In `@crates/engine/src/game/mana_sources.rs`:
- Around line 468-491: Update the SingleColor branch in the mana_types
resolution flow to avoid cloning GameState solely to set last_named_choice.
Thread the selected color into resolve_mana_types_for_ability through an
appropriate resolver argument or explicit ChoiceValue parameter, resolve the
count against &state, and preserve the existing invalid-color and per-unit color
replacement behavior.
In `@crates/engine/src/types/interaction.rs`:
- Around line 686-687: Change the action_id field in the relevant interaction
type from Option<InteractionActionId> to InteractionActionId, then update all
producers and consumers—including action_surfaces and both cancel-choice
sites—to pass and handle the value directly without Some/None or null fallback
branches.
In `@crates/phase-server/src/main.rs`:
- Around line 3779-3843: Move the per-seat StateUpdate construction, including
derive_viewer_interaction and other payload projections, out of the
connections.lock() scope by precomputing messages from ai_filtered before
acquiring the lock, then only look up recipients and send inside the lock. Add
timing instrumentation/metric for the viewer-interaction projection work so
projection regressions are observable.
🪄 Autofix (Beta)
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: 51c63e3c-9bbc-48f0-a0dc-18cc483a3779
⛔ Files ignored due to path filters (2)
Cargo.lockis excluded by!**/*.lockclient/src/adapter/generated/interaction/index.tsis excluded by!**/generated/**
📒 Files selected for processing (28)
client/src/adapter/__tests__/server-draft-adapter.test.tsclient/src/adapter/server-draft-adapter.tsclient/src/adapter/types.tsclient/src/adapter/ws-adapter.tsclient/src/i18n/locales/de/game.jsonclient/src/i18n/locales/en/game.jsonclient/src/i18n/locales/es/game.jsonclient/src/i18n/locales/fr/game.jsonclient/src/i18n/locales/it/game.jsonclient/src/i18n/locales/pl/game.jsonclient/src/i18n/locales/pt/game.jsonclient/src/network/__tests__/protocol.test.tsclient/src/network/protocol.tsclient/src/pages/GamePage.tsxclient/src/stores/gameStore.tsclient/src/stores/uiStore.tscrates/engine-wasm/src/lib.rscrates/engine/Cargo.tomlcrates/engine/src/bin/interaction_bindings.rscrates/engine/src/game/interaction.rscrates/engine/src/game/mana_abilities.rscrates/engine/src/game/mana_sources.rscrates/engine/src/types/interaction.rscrates/engine/tests/integration/interaction_contract.rscrates/lobby-broker/src/protocol.rscrates/phase-server/src/main.rscrates/server-core/src/protocol.rsscripts/check-protocol-version.mjs
There was a problem hiding this comment.
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
crates/engine/tests/integration/interaction_contract.rs (1)
1122-1157: 🎯 Functional Correctness | 🟠 Major | ⚡ Quick winAssert distinct action IDs and the complete unrestricted projection.
This test never checks
one_green != six_green; duplicate IDs could therefore pass because the first submission changes state and makes the second submission stale anyway. It also validates symbols and restrictions only for the six-green choice, so an incorrect one-green projection would remain undetected. Add explicit assertions for unique IDs and for the one-green choice’s exact{G}output with no restrictions.As per path instructions, integration tests must prove the relevant failure path and the production interaction contract, including the positive projected shape.
🤖 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 `@crates/engine/tests/integration/interaction_contract.rs` around lines 1122 - 1157, Update the assertions around the projected Castle choices and the symbols identifying them: explicitly assert that one_green and six_green have distinct action IDs, then validate one_green’s complete output shape as exactly one “G” mana symbol with no restrictions. Preserve the existing six_green restriction assertions while ensuring both projected choices’ positive shapes are fully verified.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.
Outside diff comments:
In `@crates/engine/tests/integration/interaction_contract.rs`:
- Around line 1122-1157: Update the assertions around the projected Castle
choices and the symbols identifying them: explicitly assert that one_green and
six_green have distinct action IDs, then validate one_green’s complete output
shape as exactly one “G” mana symbol with no restrictions. Preserve the existing
six_green restriction assertions while ensuring both projected choices’ positive
shapes are fully verified.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro Plus
Run ID: 5fdfeb56-b822-4b7c-9ac9-ebeac30123a9
📒 Files selected for processing (3)
crates/engine/tests/integration/interaction_contract.rscrates/phase-server/src/main.rscrates/server-core/src/protocol.rs
💤 Files with no reviewable changes (1)
- crates/server-core/src/protocol.rs
🚧 Files skipped from review as they are similar to previous changes (1)
- crates/phase-server/src/main.rs
1 participant
Summary by CodeRabbit