Skip to content

fix(client): expose targetable attachments - #6752

Merged
matthewevans merged 1 commit into
mainfrom
ship/expose-targetable-attachments
Jul 29, 2026
Merged

fix(client): expose targetable attachments#6752
matthewevans merged 1 commit into
mainfrom
ship/expose-targetable-attachments

Conversation

@matthewevans

@matthewevans matthewevans commented Jul 29, 2026

Copy link
Copy Markdown
Member

Summary by CodeRabbit

  • Bug Fixes
    • Improved attachment targeting on cards.
    • Clicking a card with actionable attachments now opens the attachment chooser, allowing users to select the intended attached card.
    • Selecting an attached card correctly proceeds with target selection.

@matthewevans
matthewevans enabled auto-merge July 29, 2026 02:44
@coderabbitai

coderabbitai Bot commented Jul 29, 2026

Copy link
Copy Markdown
Contributor

Review Change Stack

📝 Walkthrough

Walkthrough

PermanentCard now opens the attachment chooser when attached permanents are actionable, reuses a dedicated chooser callback, and tests target selection through the attachment fan.

Changes

Attachment targeting

Layer / File(s) Summary
Attachment chooser and target selection
client/src/components/board/PermanentCard.tsx, client/src/components/board/__tests__/PermanentCard.test.tsx
PermanentCard centralizes attachment fan opening and invokes it for actionable attachments; the test verifies that selecting a targetable attachment dispatches ChooseTarget with its object ID.

Estimated code review effort: 2 (Simple) | ~10 minutes

Suggested labels: bug

Suggested reviewers: lgray

🚥 Pre-merge checks | ✅ 5
✅ Passed checks (5 passed)
Check name Status Explanation
Description Check ✅ Passed Check skipped - CodeRabbit’s high-level summary is enabled.
Title check ✅ Passed The title clearly matches the change: client behavior now exposes targetable attachments through the attachment fan.
Docstring Coverage ✅ Passed No functions found in the changed files to evaluate docstring coverage. Skipping docstring coverage check.
Linked Issues check ✅ Passed Check skipped because no linked issues were found for this pull request.
Out of Scope Changes check ✅ Passed Check skipped because no linked issues were found for this pull request.
✨ Finishing Touches
📝 Generate docstrings
  • Create stacked PR
  • Commit on current branch
🧪 Generate unit tests (beta)
  • Create PR with unit tests
  • Commit unit tests in branch ship/expose-targetable-attachments

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

@coderabbitai coderabbitai Bot left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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/PermanentCard.tsx`:
- Around line 692-697: Move attachment interaction determination out of the
client: replace the client-derived attachmentsActionable routing around
showAttachmentFan with engine-provided, host-scoped interaction options and
actions. Propagate these options/actions through every adapter to AttachmentFan,
and have the fan render only engine-authorized choices and dispatch their
corresponding actions, including sacrifice, mana-cost, and undo-tap states.
Remove client-side game-data derivation so the frontend remains a display layer.
🪄 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: d3c6690f-5b53-4156-86a2-2072b8c58348

📥 Commits

Reviewing files that changed from the base of the PR and between de8dc15 and b71afd5.

📒 Files selected for processing (2)
  • client/src/components/board/PermanentCard.tsx
  • client/src/components/board/__tests__/PermanentCard.test.tsx

Comment on lines +692 to +697
} else if (attachmentsActionable) {
// The host is not a legal choice, but one of its attachments is. Open
// the full-card chooser rather than requiring a precise click on an
// overlapping attachment peek. The fan derives every selectable card
// from the engine's current legal-target set.
showAttachmentFan();

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🎯 Functional Correctness | 🟠 Major | 🏗️ Heavy lift

Move attachment actionability and actions into the engine.

Line 692 makes a client-derived attachmentsActionable predicate control gameplay routing. It also includes sacrifice, mana-cost, and undo-tap states, but AttachmentFan only handles target selection, board choices, and activation actions—so those cases open a fan where selecting the attachment performs no action. Expose engine-authorized attachment interaction options/actions per host, wire them through every adapter, and have the fan render and dispatch that state.

As per path instructions, “The frontend is a display layer, never a logic layer” and game-data derivation must be pushed into the engine.

🤖 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/PermanentCard.tsx` around lines 692 - 697, Move
attachment interaction determination out of the client: replace the
client-derived attachmentsActionable routing around showAttachmentFan with
engine-provided, host-scoped interaction options and actions. Propagate these
options/actions through every adapter to AttachmentFan, and have the fan render
only engine-authorized choices and dispatch their corresponding actions,
including sacrifice, mana-cost, and undo-tap states. Remove client-side
game-data derivation so the frontend remains a display layer.

Source: Path instructions

@matthewevans
matthewevans added this pull request to the merge queue Jul 29, 2026
Merged via the queue into main with commit 32a6a98 Jul 29, 2026
15 checks passed
@matthewevans
matthewevans deleted the ship/expose-targetable-attachments branch July 29, 2026 03:11
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant