Skip to content

fix(client): count spell colors in dominant-color detection (shard variant names)#5411

Open
boskodev790 wants to merge 1 commit into
phase-rs:mainfrom
boskodev790:fix/dominant-color-shard-variant-names
Open

fix(client): count spell colors in dominant-color detection (shard variant names)#5411
boskodev790 wants to merge 1 commit into
phase-rs:mainfrom
boskodev790:fix/dominant-color-shard-variant-names

Conversation

@boskodev790

Copy link
Copy Markdown

Problem

The dominant-color helpers in viewmodel/dominantColor.ts (getDominantManaColor, getDeckDominantColor) key their shard→color lookup by MTG symbols (W/U/B/R/G) and split each shard on "/" to catch hybrids like "W/U".

But the engine does not serialize ManaCost shards as symbols — it serializes them as Rust variant names: "White", "Red", "WhiteBlue", "PhyrexianRed", etc. This is the same convention costLabel.ts documents on SHARD_ABBREVIATION ("Converts Rust ManaCostShard variant names to MTG abbreviations") and that keywordProps.ts relies on, and it's the format used by the shared gameObjectFactory and every other viewmodel/game test (shards: ["Red"], ["White"], ["WhiteBlue"]).

Because "White".split("/") is ["White"] and SHARD_TO_COLOR["White"] is undefined, no nonland card's mana cost ever contributed a color. Dominant-color detection collapsed to a lands-only signal, so components/board/BattlefieldBackground.tsx ignored spell colors entirely (a mono-red spellslinger deck with basic Islands would tint blue).

The previous dominantColor.test.ts masked this: its two spell-cost cases fed the fake ["W","W"] / ["R"] format — the lone place in the repo disagreeing with the factory and every other test.

Fix

Bridge each shard through the canonical SHARD_ABBREVIATION map (variant name → symbol) before splitting on "/", then look up each colored half — mirroring exactly how keywordProps/costLabel already interpret shards. This makes mono, hybrid (WhiteBlueW/U → White + Blue), and Phyrexian (PhyrexianRedR/P → Red) shards all count correctly, while colorless/snow/X halves are ignored.

Tests

viewmodel/__tests__/dominantColor.test.ts:

  • Corrected the two spell-cost cases to the real engine format (["White","White"], ["Red"]). With the pre-fix symbol-keyed lookup these now return null/wrong color — proven fail-before, pass-after.
  • Added a hybrid case: ["WhiteBlue"] + ["Blue"] → Blue wins (hybrid contributes to both White and Blue).
  • Added a land+spell case where the spell color must win, which would silently mis-resolve if spell shards were dropped.

Verified with vitest: all 11 tests pass after the fix; reverting only the source change fails exactly the 3 spell-color tests (discriminating).

Model: claude-opus-4-8

…riant names)

The battlefield/deck dominant-color helpers keyed their shard lookup by MTG
symbols (W/U/B/R/G), but the engine serializes ManaCost shards as Rust variant
names ("White", "WhiteBlue", "PhyrexianRed"). Every shard lookup therefore
missed, so no nonland card's mana cost ever contributed a color — only land
subtypes counted, collapsing the tint to a lands-only signal.

Bridge shards through the canonical SHARD_ABBREVIATION map (variant name ->
symbol) before splitting hybrid/Phyrexian halves on '/', matching how
keywordProps and costLabel already interpret shards.
@boskodev790 boskodev790 requested a review from matthewevans as a code owner July 9, 2026 03:22
@gemini-code-assist

Copy link
Copy Markdown
Contributor

Warning

You have reached your daily quota limit. Please wait up to 24 hours and I will start processing your requests again!

@superagent-security superagent-security Bot added the contributor:flagged Contributor flagged for review by trust analysis. label Jul 9, 2026
@superagent-security

Copy link
Copy Markdown

🚨 Contributor flagged. Click here for more info: Superagent Dashboard

@matthewevans matthewevans added the defer-fe Frontend/client/UI PR deferred to Matt's direct review label Jul 9, 2026
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

contributor:flagged Contributor flagged for review by trust analysis. defer-fe Frontend/client/UI PR deferred to Matt's direct review

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants