fix(client): authorize attachment interaction fan - #6778
Merged
Conversation
matthewevans
enabled auto-merge
July 29, 2026 05:09
Contributor
📝 WalkthroughWalkthroughAdds attachment-fan data to the interaction contract, exposes interaction submission through WASM and client transports, and updates board components to submit viewer-scoped interaction responses. ChangesInteraction submission and attachment fan flow
Estimated code review effort: 4 (Complex) | ~45 minutes Sequence Diagram(s)sequenceDiagram
participant AttachmentFan
participant dispatchInteraction
participant EngineAdapter
participant submit_interaction_js
participant GameStore
AttachmentFan->>dispatchInteraction: submit InteractionResponse
dispatchInteraction->>EngineAdapter: submitInteraction(actor, submission)
EngineAdapter->>submit_interaction_js: apply interaction submission
submit_interaction_js-->>EngineAdapter: events and log_entries
EngineAdapter-->>dispatchInteraction: SubmitResult
dispatchInteraction->>GameStore: commit snapshot and engine events
Possibly related PRs
Suggested labels: Suggested reviewers: 🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✨ Finishing Touches📝 Generate docstrings
🧪 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}
Contributor
There was a problem hiding this comment.
Actionable comments posted: 10
🧹 Nitpick comments (1)
crates/engine-wasm/src/lib.rs (1)
1048-1048: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winReason codes reach the client only as
Debug-formatted prose.
format!("Engine error: {:?}", error.code)collapses the wholeInteractionReasonCodeenum into a string, so the UI cannot distinguish a recoverableConstraintUnsatisfied/StaleInteraction(re-derive the projection and retry) from a terminalNotAuthorized. The enum already has a camelCase TS binding — consider returning a structured{ error: { code } }envelope instead sodispatchInteractioncallers can branch.🤖 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-wasm/src/lib.rs` at line 1048, The interaction error branch in the WASM boundary should return a structured JavaScript object containing the camelCase reason code rather than a Debug-formatted string. Update the `Ok(Err(error))` handling near `dispatchInteraction` to serialize an `{ error: { code } }` envelope using the existing `InteractionReasonCode` TypeScript binding, preserving the distinct codes for retryable and terminal outcomes.
🤖 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`:
- Around line 143-153: The choiceFor callback currently collapses multiple
distinct choice IDs to choiceIds[0], preventing access to alternatives for the
same attachment object. Update the AttachmentFan choice handling to preserve and
surface all matching choice IDs, or consume an engine-provided canonical choice
ID for the affordance; ensure selection and submission use the intended distinct
choice rather than arbitrarily selecting the first.
- Line 168: Update the dispatchInteraction rejection handlers in AttachmentFan’s
submission flows to surface the failure reason to the user and clear the current
selection when the response is rejected, including ConstraintUnsatisfied,
StaleInteraction, NotAuthorized, and transport/STATE_LOST errors. Preserve close
on successful submission, but replace the swallowed catch callbacks at both
referenced flows with the existing feedback and selection-reset mechanisms.
- Around line 226-230: Update the confirm button’s styling in AttachmentFan so
the control meets the 44pt minimum touch target by increasing its vertical
padding to py-3 and/or adding min-h-11, while preserving its existing width,
typography, and disabled-state classes.
- Around line 120-124: Remove the response-spec interpretation from
requiresConfirmation and responseForChoices in AttachmentFan. Have the engine or
opportunity expose the required confirmation state and response envelope,
including choice-id encoding and unsupported cases, then make the fan only
render that state and echo selected ids into the engine-provided shape. Ensure
new InteractionResponseSpec variants cannot silently disable submission.
In `@client/src/components/board/PermanentCard.tsx`:
- Around line 466-470: Move the attachment-fan lookup out of PermanentCard and
into the game engine/adapters by exposing a host-keyed fan or actionable-host
value. Update the viewerInteraction adapter path to project that value, then
have PermanentCard consume the precomputed value for objectId without filtering
or deriving GAME state in client/src; preserve the existing actionability
behavior around the interactionAttachmentFan usage.
In `@client/src/game/dispatch.ts`:
- Around line 722-724: Update the missing-submitInteraction guard in the
interaction dispatch flow to throw the adapter’s unsupported/not-implemented
error code instead of WS_ERROR. Keep the error non-retryable and preserve the
existing capability message so recovery can select the legacy action path rather
than treating the adapter as a failed transport.
- Around line 726-731: Update dispatchInteraction around
adapter.submitInteraction and adapter.getSnapshot to use the same
STATE_LOST/panic recovery flow as processAction(), preferably through the
existing shared helper. Ensure worker failure after applying the interaction
triggers snapshot recovery so commitEngineSnapshot receives the current engine
state.
In `@crates/engine/src/game/interaction.rs`:
- Around line 7343-7401: Replace the wildcard arm in the `object_choices` match
on `HumanResponseModel` with explicit arms for every remaining enum variant.
Preserve the existing choice generation for `TargetSequence`, `Select`, and
`ExactCandidates`, and explicitly assign the intended empty or object-choice
behavior for each other variant so future enum additions produce a compiler
error.
In `@crates/engine/src/types/interaction.rs`:
- Around line 32-34: The interaction reference contract conflicts with the UI’s
numeric decoding. In crates/engine/src/types/interaction.rs:32-34, expose an
explicit ObjectId-typed field on InteractionAttachmentFan and
InteractionAttachmentFanChild while keeping InteractionObjectReference opaque;
in client/src/components/board/AttachmentFan.tsx:92-94, update the AttachmentFan
consumers to use those engine-provided object-id fields and remove
Number(fan.host)/Number(child.object) parsing.
In `@crates/engine/tests/integration/interaction_contract.rs`:
- Around line 418-432: The interaction contract test must round-trip the
published fan choice instead of only checking that choice_ids is non-empty. In
the test around viewer_interaction and the fan’s interaction_id, submit an
InteractionResponse::Select containing the first child choice ID through the
production submit_interaction pipeline, then assert the referenced attachment
becomes untapped; preserve the existing fan and child assertions.
---
Nitpick comments:
In `@crates/engine-wasm/src/lib.rs`:
- Line 1048: The interaction error branch in the WASM boundary should return a
structured JavaScript object containing the camelCase reason code rather than a
Debug-formatted string. Update the `Ok(Err(error))` handling near
`dispatchInteraction` to serialize an `{ error: { code } }` envelope using the
existing `InteractionReasonCode` TypeScript binding, preserving the distinct
codes for retryable and terminal outcomes.
🪄 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: 8a0a01f8-03d7-427a-8fe0-c05ee044205a
⛔ Files ignored due to path filters (2)
client/src/adapter/generated/interaction/index.tsis excluded by!**/generated/**client/src/wasm/engine_wasm.d.tsis excluded by!client/src/wasm/**,!**/*.d.ts
📒 Files selected for processing (18)
client/src/adapter/__tests__/server-draft-adapter.test.tsclient/src/adapter/engine-worker-client.tsclient/src/adapter/engine-worker.tsclient/src/adapter/p2p-adapter.tsclient/src/adapter/server-draft-adapter.tsclient/src/adapter/types.tsclient/src/adapter/wasm-adapter.tsclient/src/adapter/ws-adapter.tsclient/src/components/board/AttachmentFan.tsxclient/src/components/board/PermanentCard.tsxclient/src/game/dispatch.tsclient/src/network/protocol.tsclient/src/pages/__tests__/GamePage.projectedManaChoices.test.tscrates/engine-wasm/src/lib.rscrates/engine/src/bin/interaction_bindings.rscrates/engine/src/game/interaction.rscrates/engine/src/types/interaction.rscrates/engine/tests/integration/interaction_contract.rs
Comment on lines
+726
to
+731
| const result = await adapter.submitInteraction(submission, actor); | ||
| const snapshot = await adapter.getSnapshot(); | ||
| useGameStore.getState().commitEngineSnapshot(snapshot, { | ||
| events: result.events, | ||
| logEntries: result.log_entries ?? [], | ||
| }); |
Contributor
There was a problem hiding this comment.
🩺 Stability & Availability | 🟠 Major | ⚡ Quick win
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
ast-grep run --pattern 'async function dispatchActionInternal($$$) { $$$ }' --lang typescript client/src/game/dispatch.ts
rg -nP -C4 '(commitEngineSnapshot|saveGame|STATE_LOST|recover)' client/src/game/dispatch.ts | head -60Repository: phase-rs/phase
Length of output: 3207
🏁 Script executed:
#!/bin/bash
set -euo pipefail
# Map the file and inspect the relevant sections around the cited lines and the internal dispatch path.
wc -l client/src/game/dispatch.ts
sed -n '280,390p' client/src/game/dispatch.ts
printf '\n---\n'
sed -n '700,760p' client/src/game/dispatch.ts
printf '\n---\n'
rg -n 'dispatchActionInternal|submitInteraction|getSnapshot|commitEngineSnapshot|attemptStateRehydrate|STATE_LOST|notifyEngineLost|isStateLost' client/src/game/dispatch.tsRepository: phase-rs/phase
Length of output: 10390
🏁 Script executed:
#!/bin/bash
set -euo pipefail
# Inspect related adapter behavior to determine whether submitInteraction mutates authoritative state
# before getSnapshot, and whether any caller wraps this path with recovery.
rg -n 'submitInteraction|submitAction|getSnapshot|restoreState|STATE_LOST|commitEngineSnapshot' client/src -g '!**/*.map'Repository: phase-rs/phase
Length of output: 41450
🏁 Script executed:
#!/bin/bash
set -euo pipefail
# Inspect the adapter API definition and the caller that uses dispatchActionInternal.
ast-grep outline client/src/game/dispatch.ts --match dispatchActionInternal --view expanded
printf '\n---\n'
ast-grep outline client/src -g 'client/src/**/*.ts' --match submitInteraction --view expandedRepository: phase-rs/phase
Length of output: 395
🏁 Script executed:
#!/bin/bash
set -euo pipefail
sed -n '650,770p' client/src/game/dispatch.ts | cat -nRepository: phase-rs/phase
Length of output: 5147
🏁 Script executed:
#!/bin/bash
set -euo pipefail
# Find all submitInteraction call sites and any surrounding error handling.
rg -n -C 6 'submitInteraction\(' client/src
printf '\n---\n'
# Find getSnapshot call sites, especially ones wrapped in try/catch or recovery logic.
rg -n -C 6 'getSnapshot\(' client/src/game/dispatch.ts client/srcRepository: phase-rs/phase
Length of output: 48351
Handle getSnapshot() recovery in dispatchInteraction(). This path submits the interaction, then reads a snapshot with no STATE_LOST/panic retry. If the worker dies after applying the interaction, the store keeps the old snapshot and the UI diverges from engine state. Reuse the same recovery flow as processAction(), or route this through that shared helper.
🤖 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/game/dispatch.ts` around lines 726 - 731, Update
dispatchInteraction around adapter.submitInteraction and adapter.getSnapshot to
use the same STATE_LOST/panic recovery flow as processAction(), preferably
through the existing shared helper. Ensure worker failure after applying the
interaction triggers snapshot recovery so commitEngineSnapshot receives the
current engine state.
Contributor
There was a problem hiding this comment.
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/board/__tests__/PermanentCard.test.tsx`:
- Around line 149-182: Extend interactionForAttachedObject and the related
PermanentCard tests to support a fan with multiple children and distinct
choiceIds, rather than only a single object 4 fixture. Add coverage that clicks
each child and verifies its corresponding choiceId is submitted, preserving the
existing single-child behavior while exercising child/choice association across
the full fixture range.
🪄 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: b09b3e19-8dd6-4624-8d50-c0ab0f962543
📒 Files selected for processing (2)
client/src/components/board/__tests__/PermanentCard.test.tsxclient/src/network/__tests__/protocol.test.ts
Contributor
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 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`:
- Around line 158-160: Update the selectable prop on the attachment fan card in
AttachmentFan to also require viewerInteraction?.canSubmit, while preserving the
existing interactionFan and host-id checks so unauthorized or waiting viewers
see a read-only fan.
- Around line 205-210: Update the selectable attachment card around the
motion.div click handler to be keyboard-operable and semantically interactive:
use a button element when selectable, or add appropriate button semantics,
focusability, and Enter/Space activation while preserving stopPropagation and
onPick(objectId) behavior.
🪄 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: 4ee6fe16-04f7-4973-8a38-022ca0e36361
⛔ Files ignored due to path filters (1)
client/src/adapter/generated/interaction/index.tsis excluded by!**/generated/**
📒 Files selected for processing (10)
client/src/adapter/__tests__/server-draft-adapter.test.tsclient/src/adapter/types.tsclient/src/components/board/AttachmentFan.tsxclient/src/components/board/PermanentCard.tsxclient/src/components/board/__tests__/PermanentCard.test.tsxclient/src/game/dispatch.tsclient/src/pages/__tests__/GamePage.projectedManaChoices.test.tscrates/engine/src/game/interaction.rscrates/engine/src/types/interaction.rscrates/engine/tests/integration/interaction_contract.rs
🚧 Files skipped from review as they are similar to previous changes (9)
- client/src/adapter/tests/server-draft-adapter.test.ts
- client/src/pages/tests/GamePage.projectedManaChoices.test.ts
- client/src/adapter/types.ts
- crates/engine/src/types/interaction.rs
- client/src/game/dispatch.ts
- crates/engine/tests/integration/interaction_contract.rs
- client/src/components/board/PermanentCard.tsx
- client/src/components/board/tests/PermanentCard.test.tsx
- crates/engine/src/game/interaction.rs
Comment on lines
+158
to
160
| selectable={interactionFan !== null && id !== host.id} | ||
| onPick={handlePick} | ||
| /> |
Contributor
There was a problem hiding this comment.
🎯 Functional Correctness | 🟠 Major | ⚡ Quick win
Gate the pick affordance on canSubmit.
When canSubmit is false, these cards still appear selectable, but Lines 113-114 silently ignore the click. Include viewerInteraction?.canSubmit in selectable so unauthorized/waiting viewers see a read-only fan.
🤖 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 158 - 160, Update
the selectable prop on the attachment fan card in AttachmentFan to also require
viewerInteraction?.canSubmit, while preserving the existing interactionFan and
host-id checks so unauthorized or waiting viewers see a read-only fan.
Comment on lines
205
to
210
| onClick={(e) => { | ||
| e.stopPropagation(); | ||
| if (selectable) onPick(objectId, choice); | ||
| if (selectable) onPick(objectId); | ||
| }} | ||
| aria-label={obj.name} | ||
| className={`relative leading-[0] select-none ${selectable ? "cursor-pointer" : "cursor-default"}`} |
Contributor
There was a problem hiding this comment.
🎯 Functional Correctness | 🟠 Major | ⚡ Quick win
Make selectable cards keyboard-operable.
The clickable motion.div is not focusable or semantically interactive; keyboard users cannot submit an attachment choice. Use a semantic button for selectable cards, or add button semantics plus Enter/Space handling.
🤖 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 205 - 210, Update
the selectable attachment card around the motion.div click handler to be
keyboard-operable and semantically interactive: use a button element when
selectable, or add appropriate button semantics, focusability, and Enter/Space
activation while preserving stopPropagation and onPick(objectId) behavior.
This was referenced Aug 3, 2026
matthewevans
added a commit
to tp5grphwkt-cell/phase
that referenced
this pull request
Aug 3, 2026
…6941) (phase-rs#6949) * fix(server): accept Interaction submissions over WebSocket (phase-rs#6941) `ClientMessage` had no `Interaction` variant, so the server rejected every interaction submission at serde deserialization -- before any game logic ran -- with "unknown variant `Interaction`". PR phase-rs#6778 wired the engine, WASM, and client halves of the attachment interaction fan but shipped no server half (`git show --stat a876682 -- crates/server-core` is empty). Live since v0.42.0: every interaction submission in a WebSocket multiplayer game failed. The client envelope was already correct -- `ClientMessage` is `#[serde(tag = "type", content = "data")]`, matching what `ws-adapter.ts` sends -- so no client file changes. - Add `ClientMessage::Interaction { submission }` and declare its wire policy in all three exhaustive `ClientMessage` matches. - Add `SessionManager::handle_interaction`, deriving the acting seat from the authenticated session token, never from the payload. `submit_interaction` then re-authorizes against the interaction slot inside the engine, so a forged id belonging to another seat is rejected twice. - Extract the `ClientMessage::Action` handler body into `handle_full_game_submission` so both wire variants share one authenticated, applied, and broadcast path rather than forking eight fan-out behaviours. - Answer interaction bounds failures on `ServerMessage::ActionRejected`, not `ServerMessage::Error`. The native client disposes its adapter on any `Error`, and a free-form `Text` response exceeding the 256-byte bound is reachable by an ordinary paste -- routing it to `Error` would end the match. `wire_rejection_message` makes that channel a per-variant wire policy. - Promote the engine's existing response bounds to a public `bound_interaction_submission` so the wire invokes the engine's own limits instead of restating them. Server-hosted draft matches remain blocked on a separate identity-binding defect (the draft adapter drops `DraftMatchStart.player_token`), which needs a client change and is filed separately. * fix(server): label shared submission diagnostics by kind `handle_full_game_submission` serves both `GameSubmission::Action` and `GameSubmission::Interaction`, but two of its events still named only the action path -- so an operator triaging an interaction-submission report would grep for "interaction" and match neither the "not in a game" warning nor the "processed (lock held)" info event. Derive a `kind` label from the submission variant and attach it to both, renaming the messages to name the shared concept. `GameSubmission::kind` keeps the two call sites from restating the variant set. Raised by CodeRabbit on phase-rs#6949. * docs(server-core): correct the wire-guard module's match count The module header claimed this layer "guarantees a single exhaustive match". It has had two for some time and now has three, and the doc on `wire_rejection_message` already referred to "the two sibling matches in this module" -- contradicting the header it sits under. State the guarantee in terms of what it actually enforces (every wire policy is declared in an exhaustive, wildcard-free match, so a new variant cannot compile until it states one) and name the three policy axes by function rather than by count, so the header does not go stale again the next time one is added. Raised by review on phase-rs#6949. Doc-only; no behavior change. --------- Co-authored-by: matthewevans <matthewevans@users.noreply.github.com>
This was referenced Aug 4, 2026
4 tasks
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary by CodeRabbit